[PATCH v3 00/12] MBus device tree binding

[PATCH v3 00/12] MBus device tree binding

[Ezequiel Garcia]

In the current device tree binding proposal, for each MBus' child device
there's an mbus-node 'ranges' property entry, in addition to the ranges
property of the child itself.

For each of this mbus-node ranges entry, the MBus driver creates a decoding
window for it, on probe time. For instance, in the example below the
BootROM child is specified:

	soc {
		compatible = "marvell,armada370-mbus", "simple-bus";
		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
		#address-cells = <2>;
		#size-cells = <1>;

		/* The window base address is encoded in this translation */
		ranges = < ... /* other entries */
			   0x011d0000 0 0 0xfff00000 0x100000>;

		bootrom {
			#address-cells = <1>;
			#size-cells = <1>;
			ranges = <0 0x011d0000 0 0xffffffff>;
		};
	};

Notice that, since the decoding windows are allocated using the base address
and size declared in the mbus-node, it's possible to declare a 'ranges'
translation on the entire window maximum size in the children nodes.
This simplifies a bit the representation, since it concentrates the relevant
information in only one place.

The target ID and attribute are encoding in the first cell of the MBus
address space. The lower bytes are 0x0000 for entries that correspond
directly to decoding windows.

Since we also need to have valid translations for the internal register
space and the PCIe space, yet these translations do not correspond to
windows that need allocation, we choose to put a non-zero value on the
lower bytes (using 0xffff as target ID and attribute). This way the binding
skips those entries, for they don't correspond to valid windows.

In the example below there's an extract of a device tree showing how
the internal-regs and pcie nodes can be represented:

	#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))

	soc {
		compatible = "marvell,armadaxp-mbus";
		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;

		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
			  0xffff0000 0 0 0xe0000000 0x8100000  /* pcie */
			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;

		bootrom {
			#address-cells = <1>;
			#size-cells = <1>;
		};

		devbus-bootcs {
			status = "okay";

			/* NOR */
			nor {
				compatible = "cfi-flash";
				reg = <0 0x8000000>;
				bank-width = <2>;
			};
		};

		pcie-controller {
			compatible = "marvell,armada-xp-pcie";
			status = "okay";
			device_type = "pci";

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

			ranges =
			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
				0x82000000 0 0xe0000000 0xffff0000 0 0 0x08000000   /* non-prefetchable memory */
				0x81000000 0 0 0xffff0000 0x8000000 0 0x00100000>; /* downstream I/O */

			pcie at 1,0 {
				/* Port 0, Lane 0 */
				status = "okay";
			};
		};

		internal-regs {
			compatible = "simple-bus";
			#address-cells = <1>;
			#size-cells = <1>;
			...
		};
	};

In this current patchset I think we've solved every issue, so I'd like to get
some acknowledgment for inclusion in v3.11. If there's anything left to fix
or review, I'd be happy to do so!

This patchset is based on v3.10-rc4, with Jason Cooper's mvebu/regmap branch
applied, and with the mvebu-devbus commit (3edad321b1 in linux-next):
"drivers: memory: Introduce Marvell EBU Device Bus driver"

Changes from v2:
 * Replaced the PCIe mapping with 0xffff0002, to avoid using a representation
   that might correspond to a possible window id.

 * Remove every mbus-node 'ranges' property from the dtsi files. Having them
   on the per-board dts files only makes maintaince less painful.

 * Declare children window size, in the children 'ranges' property, as large
   as possible (4 GiB size) and move the property to the dtsi files.
   The per-board dts, does not need to declare that property now.
 
 * The MBus driver now creates the decoding window using the base address and
   size specified in the mbus-node, instead of using the children entries.

Change from v1:

 * Use the preprocessor on the DT and define a macro for the window ID.

 * Changed the way the address spaces were declared: the window's base
   addresses are only present in the mbus-node ranges property.
   This makes sense since that information belongs only to the MBus
   address space.

 * Drop the ranges dynamic update, since now the DT translations are complete.
 
 * Replaced the internal register first cell mapping with 0xffff0001, to avoid
   clashing when using 0x00000000.

 * Replaced the PCIe mapping with 0xffff0000.

Thanks a lot!

Ezequiel Garcia (12):
  bus: mvebu-mbus: Factor out initialization details
  bus: mvebu-mbus: Introduce device tree binding
  bus: mvebu-mbus: Add static window allocation to the DT binding
  ARM: mvebu: Initialize MBus using the DT binding
  ARM: mvebu: Remove the harcoded BootROM window allocation
  memory: mvebu-devbus: Remove address decoding window workaround
  ARM: mvebu: Use the preprocessor on Armada 370/XP device tree files
  ARM: mvebu: Add MBus to Armada 370/XP device tree
  ARM: mvebu: Add BootROM to Armada 370/XP device tree
  ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes
  ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
  ARM: mvebu: Relocate Armada XP PCIe device tree nodes

 .../devicetree/bindings/bus/mvebu-mbus.txt         | 168 +++++++++
 arch/arm/boot/dts/armada-370-db.dts                |   6 +-
 arch/arm/boot/dts/armada-370-mirabox.dts           |  38 ++-
 arch/arm/boot/dts/armada-370-rd.dts                |   6 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               | 103 +++---
 arch/arm/boot/dts/armada-370.dtsi                  | 110 +++---
 arch/arm/boot/dts/armada-xp-db.dts                 |  72 ++--
 arch/arm/boot/dts/armada-xp-gp.dts                 | 107 +++---
 arch/arm/boot/dts/armada-xp-mv78230.dtsi           | 209 ++++++------
 arch/arm/boot/dts/armada-xp-mv78260.dtsi           | 247 +++++++-------
 arch/arm/boot/dts/armada-xp-mv78460.dtsi           | 379 +++++++++++----------
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts   |  83 ++---
 arch/arm/boot/dts/armada-xp.dtsi                   |  11 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |  34 +-
 arch/arm/mach-mvebu/platsmp.c                      |   1 -
 drivers/bus/mvebu-mbus.c                           | 196 ++++++++++-
 drivers/memory/mvebu-devbus.c                      |  64 +---
 include/linux/mbus.h                               |   1 +
 18 files changed, 1061 insertions(+), 774 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/mvebu-mbus.txt

-- 
1.8.1.5

[PATCH v3 01/12] bus: mvebu-mbus: Factor out initialization details

[Ezequiel Garcia]

We introduce a common initialization function mvebu_mbus_common_init()
that will be used by both legacy and device-tree initialization code.
This patch is an intermediate step, which will allow to introduce the
DT binding for this driver in a less intrusive way.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/bus/mvebu-mbus.c | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 33c6947..d0feee5 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -830,26 +830,14 @@ static __init int mvebu_mbus_debugfs_init(void)
 }
 fs_initcall(mvebu_mbus_debugfs_init);
 
-int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
-			   size_t mbuswins_size,
-			   phys_addr_t sdramwins_phys_base,
-			   size_t sdramwins_size)
+static int __init mvebu_mbus_common_init(struct mvebu_mbus_state *mbus,
+					 phys_addr_t mbuswins_phys_base,
+					 size_t mbuswins_size,
+					 phys_addr_t sdramwins_phys_base,
+					 size_t sdramwins_size)
 {
-	struct mvebu_mbus_state *mbus = &mbus_state;
-	const struct of_device_id *of_id;
 	int win;
 
-	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
-		if (!strcmp(of_id->compatible, soc))
-			break;
-
-	if (!of_id->compatible) {
-		pr_err("could not find a matching SoC family\n");
-		return -ENODEV;
-	}
-
-	mbus->soc = of_id->data;
-
 	mbus->mbuswins_base = ioremap(mbuswins_phys_base, mbuswins_size);
 	if (!mbus->mbuswins_base)
 		return -ENOMEM;
@@ -870,3 +858,28 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 
 	return 0;
 }
+
+int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
+			   size_t mbuswins_size,
+			   phys_addr_t sdramwins_phys_base,
+			   size_t sdramwins_size)
+{
+	const struct of_device_id *of_id;
+
+	for (of_id = of_mvebu_mbus_ids; of_id->compatible; of_id++)
+		if (!strcmp(of_id->compatible, soc))
+			break;
+
+	if (!of_id->compatible) {
+		pr_err("could not find a matching SoC family\n");
+		return -ENODEV;
+	}
+
+	mbus_state.soc = of_id->data;
+
+	return mvebu_mbus_common_init(&mbus_state,
+			mbuswins_phys_base,
+			mbuswins_size,
+			sdramwins_phys_base,
+			sdramwins_size);
+}
-- 
1.8.1.5

[PATCH v3 02/12] bus: mvebu-mbus: Introduce device tree binding

[Ezequiel Garcia]

This patch adds the most fundamental device-tree initialization.
We only introduce what's required to be able to probe the mvebu-mbus
driver from the DT. Follow-up patches will extend the device tree binding,
allowing to describe static address decoding windows.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/bus/mvebu-mbus.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/mbus.h     |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index d0feee5..23f6ae6 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -883,3 +883,39 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 			sdramwins_phys_base,
 			sdramwins_size);
 }
+
+#ifdef CONFIG_OF
+int __init mvebu_mbus_dt_init(void)
+{
+	struct resource mbuswins_res, sdramwins_res;
+	struct device_node *np;
+	const struct of_device_id *of_id;
+	int ret;
+
+	np = of_find_matching_node(NULL, of_mvebu_mbus_ids);
+	if (!np) {
+		pr_err("could not find a matching SoC family\n");
+		return -ENODEV;
+	}
+
+	of_id = of_match_node(of_mvebu_mbus_ids, np);
+	mbus_state.soc = of_id->data;
+
+	if (of_address_to_resource(np, 0, &mbuswins_res)) {
+		pr_err("cannot get MBUS register address\n");
+		return -EINVAL;
+	}
+
+	if (of_address_to_resource(np, 1, &sdramwins_res)) {
+		pr_err("cannot get SDRAM register address\n");
+		return -EINVAL;
+	}
+
+	ret = mvebu_mbus_common_init(&mbus_state,
+				     mbuswins_res.start,
+				     resource_size(&mbuswins_res),
+				     sdramwins_res.start,
+				     resource_size(&sdramwins_res));
+	return ret;
+}
+#endif
diff --git a/include/linux/mbus.h b/include/linux/mbus.h
index dba482e..a93c62b 100644
--- a/include/linux/mbus.h
+++ b/include/linux/mbus.h
@@ -68,5 +68,6 @@ int mvebu_mbus_del_window(phys_addr_t base, size_t size);
 int mvebu_mbus_init(const char *soc, phys_addr_t mbus_phys_base,
 		    size_t mbus_size, phys_addr_t sdram_phys_base,
 		    size_t sdram_size);
+int mvebu_mbus_dt_init(void);
 
 #endif /* __LINUX_MBUS_H */
-- 
1.8.1.5

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

This patch adds static window allocation to the device tree binding.
Each first-child of the mbus-compatible node, with a suitable 'ranges'
property, declaring an address translation, will trigger an address
decoding window allocation.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../devicetree/bindings/bus/mvebu-mbus.txt         | 168 +++++++++++++++++++++
 drivers/bus/mvebu-mbus.c                           | 121 ++++++++++++++-
 2 files changed, 288 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/bus/mvebu-mbus.txt

diff --git a/Documentation/devicetree/bindings/bus/mvebu-mbus.txt b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
new file mode 100644
index 0000000..e15c280
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
@@ -0,0 +1,168 @@
+
+* Marvell MBus controller
+
+Required properties:
+
+- compatible:	 Should be set to one of the following:
+		 marvell,armada370-mbus
+		 marvell,armadaxp-mbus
+
+- reg:		 Device's register space.
+		 Two entries are expected, see the examples below.
+		 The first one controls the devices decoding window and
+		 the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+                 the second cell for the address offset within the window.
+
+- size-cells:    Must be '1'.
+
+- ranges:        Must be set up to provide a proper translation for each child.
+	         See the examples below.
+
+Example:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+	};
+
+** MBus child device specification
+
+Each child device needs at least a 'ranges' property. If the child is avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding window
+for it. For instance, in the example below the BootROM child is specified:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		ranges = < ... /* other entries */
+			   0x011d0000 0 0 0xfff00000 0x100000>;
+
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x011d0000 0 0x100000>;
+		};
+
+		/* other children */
+		...
+	};
+
+In the shown example, the MBus child node together with the translation
+entry in the 'ranges' property is what makes the MBus driver creates
+a static decoding window for the given child device.
+
+Since each window is identified by its target ID and attribute ID there's
+a special macro that can be use to simplify the translation entries:
+
+#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))
+
+Using this macro, the bootrom child node can be written in a slightly
+more readable fashion:
+
+	bootrom {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 MBUS_ID(0x01, 0x1d) 0 0x100000>;
+	};
+
+** About the target ID and attribute encodig
+
+As stated above, for each mbus-node first-level child, the MBus driver will
+allocate a decoding window. The window target ID and attribute is created
+using the first cell, which must have the following format:
+
+IIAA0000
+
+Where:
+  -- I = Marvell defined target ID for programmable windows
+  -- A = Marvell defined target attributes for programmable windows
+
+Valid windows are required to define the lower bytes as zero.
+Entries that do not correspond to valid windows, and must be skipped by
+the MBus driver, set a non-zero value in the lower bytes.
+
+** About the window base address
+
+Remember the MBus controller allows a great deal of flexibility for choosing
+the decoding window base address. When planning the device tree layout it's
+possible to choose any address as the base address, provided of course there's
+a region large enough available, and with the required alignment.
+
+Yet in other words: there's nothing preventing us from setting a base address
+of 0xf0000000, or 0xd0000000 for the NOR device shown above, if such region is
+unused.
+
+** Example
+
+See the example below, where a more complete device tree is shown:
+
+	soc {
+		compatible = "marvell,armadaxp-mbus";
+		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
+
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
+			  0xffff0002 0 0 0xe0000000 0x8100000  /* pcie */
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;
+
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 MBUS_ID(0x01, 0x1d) 0 0x100000>;
+		};
+
+		devbus-bootcs {
+			status = "okay";
+			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0x8000000>;
+
+			/* NOR */
+			nor {
+				compatible = "cfi-flash";
+				reg = <0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
+
+		pcie-controller {
+			compatible = "marvell,armada-xp-pcie";
+			status = "okay";
+			device_type = "pci";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			ranges =
+			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
+				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
+				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie at 1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+		};
+
+		internal-regs {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0xffff0001 0 0x100000>;
+
+			interrupt-controller at 20000 {
+			      reg = <0x20a00 0x2d0>, <0x21070 0x58>;
+			};
+		};
+	};
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 23f6ae6..4f086c7 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -312,6 +312,7 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
 		writel(0, addr + WIN_REMAP_HI_OFF);
 	}
 
+	pr_info("window setup: %x:%x, 0x%x@%x\n", target, attr, base, size);
 	return 0;
 }
 
@@ -885,6 +886,120 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 }
 
 #ifdef CONFIG_OF
+/*
+ * The window IDs in the ranges DT property have the following format:
+ *  - bits 24 to 31: window target ID
+ *  - bits 16 to 23: window attribute ID
+ *  - bits  8 to 15: unused
+ *  - bits  0 to 7:  custom modifiers
+ */
+#define TARGET(id) (((id) & 0xFF000000) >> 24)
+#define ATTR(id)   (((id) & 0x00FF0000) >> 16)
+#define CUSTOM(id) (((id) & 0x000000FF))
+
+static int __init mbus_dt_setup_win(struct mvebu_mbus_state *mbus,
+				    u32 base, u32 size,
+				    u8 target, u8 attr)
+{
+	const struct mvebu_mbus_mapping *map = mbus->soc->map;
+	const char *name;
+	int i;
+
+	/* Search for a suitable window in the existing mappings */
+	for (i = 0; map[i].name; i++)
+		if (map[i].target == target &&
+		    map[i].attr == (attr & map[i].attrmask))
+			break;
+
+	name = map[i].name;
+	if (!name) {
+		pr_err("window 0x%x:0x%x is unknown, skipping\n",
+		       target, attr);
+		return -EINVAL;
+	}
+
+	if (!mvebu_mbus_window_conflicts(mbus, base, size, target, attr)) {
+		pr_err("cannot add window '%s', conflicts with another window\n",
+		       name);
+		return -EBUSY;
+	}
+
+	if (mvebu_mbus_alloc_window(mbus, base, size, MVEBU_MBUS_NO_REMAP,
+				    target, attr)) {
+		pr_err("cannot add window '%s', too many windows\n",
+		       name);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static int __init
+mbus_parse_ranges(struct device_node *node,
+		  int *addr_cells, int *c_addr_cells, int *c_size_cells,
+		  int *cell_count, const __be32 **ranges_start,
+		  const __be32 **ranges_end)
+{
+	const __be32 *prop;
+	int ranges_len, tuple_len;
+
+	*addr_cells = of_n_addr_cells(node);
+
+	prop = of_get_property(node, "#address-cells", NULL);
+	*c_addr_cells = be32_to_cpup(prop);
+
+	prop = of_get_property(node, "#size-cells", NULL);
+	*c_size_cells = be32_to_cpup(prop);
+
+	*cell_count = *addr_cells + *c_addr_cells + *c_size_cells;
+	tuple_len = (*cell_count) * sizeof(__be32);
+
+	*ranges_start = of_get_property(node, "ranges", &ranges_len);
+	*ranges_end = *ranges_start + ranges_len / sizeof(__be32);
+
+	if (*ranges_start == NULL || ranges_len % tuple_len) {
+		pr_warn("malformed ranges entry '%s'\n", node->name);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus,
+				struct device_node *np)
+{
+	int addr_cells, c_addr_cells, c_size_cells;
+	int i, ret, cell_count;
+	const __be32 *r, *ranges_start, *ranges_end;
+
+	ret = mbus_parse_ranges(np, &addr_cells, &c_addr_cells,
+				&c_size_cells, &cell_count,
+				&ranges_start, &ranges_end);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0, r = ranges_start; r < ranges_end; r += cell_count, i++) {
+		u32 windowid, base, size;
+		u8 target, attr;
+
+		windowid = of_read_number(r, 1);
+		/*
+		 * An entry with a non-zero custom field do not
+		 * correspond to a static window, skip it.
+		 */
+		if (CUSTOM(windowid))
+			continue;
+		target = TARGET(windowid);
+		attr = ATTR(windowid);
+
+		base = of_read_number(r + c_addr_cells, addr_cells);
+		size = of_read_number(r + c_addr_cells + addr_cells,
+				      c_size_cells);
+		ret = mbus_dt_setup_win(mbus, base, size, target, attr);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 int __init mvebu_mbus_dt_init(void)
 {
 	struct resource mbuswins_res, sdramwins_res;
@@ -916,6 +1031,10 @@ int __init mvebu_mbus_dt_init(void)
 				     resource_size(&mbuswins_res),
 				     sdramwins_res.start,
 				     resource_size(&sdramwins_res));
-	return ret;
+	if (ret)
+		return ret;
+
+	/* Setup statically declared windows in the DT */
+	return mbus_dt_setup(&mbus_state, np);
 }
 #endif
-- 
1.8.1.5

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> +Required properties:
> +
> +- compatible:	 Should be set to one of the following:
> +		 marvell,armada370-mbus
> +		 marvell,armadaxp-mbus
> +
> +- reg:		 Device's register space.
> +		 Two entries are expected, see the examples below.
> +		 The first one controls the devices decoding window and
> +		 the second one controls the SDRAM decoding window.
> +
> +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> +                 the second cell for the address offset within the window.
> +
> +- size-cells:    Must be '1'.
> +
> +- ranges:        Must be set up to provide a proper translation for each child.
> +	         See the examples below.

You should explain here what the policy is regarding windows set up by the
boot loader. Are the ranges in here required to be the ones that the boot
loader has preconfigured, or are they the ones that the mbus driver is supposed
to set up?

> +Each child device needs at least a 'ranges' property. If the child is avaiable
> +(i.e. status not 'disabled'), then the MBus driver creates a decoding window
> +for it. For instance, in the example below the BootROM child is specified:
> +
> +	soc {
> +		compatible = "marvell,armada370-mbus", "simple-bus";
> +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +
> +		ranges = < ... /* other entries */
> +			   0x011d0000 0 0 0xfff00000 0x100000>;
> +
> +		bootrom {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0 0x011d0000 0 0x100000>;
> +		};
> +
> +		/* other children */
> +		...
> +	};

Do you really want to require the child to provide a "ranges" property?
I think this makes it more complicated to specify devices that belong
into the "internal-regs" category.

Is this mainly for convenience when settup up the windows?

> +
> +			ranges =
> +			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
> +				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
> +				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
> +				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
> +				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
> +				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
> +				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
> +				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
> +				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */

Using 0xffff0002 as a placeholder for the pcie translation is definitely
better than 0xffff0000 as you had before, but let me ask again in case
you missed it the last time (and sorry if I missed the answer):

Why not just put the actual translation here the way it happens for each
of the PCIe ports? With the definition here, the PCIe driver actually has no
way to figure out what settings the windows need to use!

On a side note, is there a reason why you use 0xffff0001 for the internal-regs
rather than just 0x1? This one isn't important as they both work as well, it's
just more to write your way.

	Arnd

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 18 Jun 2013 18:14:33 +0200, Arnd Bergmann wrote:

> Using 0xffff0002 as a placeholder for the pcie translation is definitely
> better than 0xffff0000 as you had before, but let me ask again in case
> you missed it the last time (and sorry if I missed the answer):
> 
> Why not just put the actual translation here the way it happens for each
> of the PCIe ports? With the definition here, the PCIe driver actually has no
> way to figure out what settings the windows need to use!

Come on Arnd, this is something we have already discussed *countless*
times with you.

We *cannot* define translations for each PCIe port because we don't
know in advance how much I/O and memory space each PCIe device will
request. This is the reason why we have *one* global range for I/O
space and *one* global space for memory space, that are given to the
Linux PCI core, which then dynamically assigns sub-ranges for each
PCIe device into those two global ranges.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 18 June 2013, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
> 
> On Tue, 18 Jun 2013 18:14:33 +0200, Arnd Bergmann wrote:
> 
> > Using 0xffff0002 as a placeholder for the pcie translation is definitely
> > better than 0xffff0000 as you had before, but let me ask again in case
> > you missed it the last time (and sorry if I missed the answer):
> > 
> > Why not just put the actual translation here the way it happens for each
> > of the PCIe ports? With the definition here, the PCIe driver actually has no
> > way to figure out what settings the windows need to use!
> 
> Come on Arnd, this is something we have already discussed countless
> times with you.
> 
> We cannot define translations for each PCIe port because we don't
> know in advance how much I/O and memory space each PCIe device will
> request. This is the reason why we have one global range for I/O
> space and one global space for memory space, that are given to the
> Linux PCI core, which then dynamically assigns sub-ranges for each
> PCIe device into those two global ranges.

Could you just stop assuming that I'm an idiot and read my suggestion?

The information that this puts into the device-tree is not where things
are mapped by how things are connected in hardware. The mbus mapping
is the part that is variable, while the PCI mapping is actually fixed
in hardware.

	Arnd

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Arnd,

On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > +Required properties:
> > +
> > +- compatible:	 Should be set to one of the following:
> > +		 marvell,armada370-mbus
> > +		 marvell,armadaxp-mbus
> > +
> > +- reg:		 Device's register space.
> > +		 Two entries are expected, see the examples below.
> > +		 The first one controls the devices decoding window and
> > +		 the second one controls the SDRAM decoding window.
> > +
> > +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> > +                 the second cell for the address offset within the window.
> > +
> > +- size-cells:    Must be '1'.
> > +
> > +- ranges:        Must be set up to provide a proper translation for each child.
> > +	         See the examples below.
> 
> You should explain here what the policy is regarding windows set up by the
> boot loader. Are the ranges in here required to be the ones that the boot
> loader has preconfigured, or are they the ones that the mbus driver is supposed
> to set up?
> 

Actually, I might be confused but the kernel MBus driver is completely
independent from the bootloader's -except in the internal register case,
(because we don't touch that from where the bootloader left it).

What happens is that any decoding window that was setup by the bootloader,
is wiped and completely new windows are allocated using the translations
in the DT, as described by this binding.

This was the case from the start with the old MBus driver. FWIW, I think
it's actually the best choice that can be made: it makes the kernel
independent of the previous setting.

I know you've suggested differently in the past, but I'm not sure I
understand what's the benefit in keeping the bootloaders configuration.

> > +Each child device needs at least a 'ranges' property. If the child is avaiable
> > +(i.e. status not 'disabled'), then the MBus driver creates a decoding window
> > +for it. For instance, in the example below the BootROM child is specified:
> > +
> > +	soc {
> > +		compatible = "marvell,armada370-mbus", "simple-bus";
> > +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +
> > +		ranges = < ... /* other entries */
> > +			   0x011d0000 0 0 0xfff00000 0x100000>;
> > +
> > +		bootrom {
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges = <0 0x011d0000 0 0x100000>;
> > +		};
> > +
> > +		/* other children */
> > +		...
> > +	};
> 
> Do you really want to require the child to provide a "ranges" property?
> I think this makes it more complicated to specify devices that belong
> into the "internal-regs" category.
> 

No, this text is actually a left-over from the previous patchset, in
current v3 patchset MBus children are *not* required to have any ranges.
On the otherside, although you will need one except in the most trivial
cases like for the BootROM.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > +Required properties:
> > > +
> > > +- compatible:	 Should be set to one of the following:
> > > +		 marvell,armada370-mbus
> > > +		 marvell,armadaxp-mbus
> > > +
> > > +- reg:		 Device's register space.
> > > +		 Two entries are expected, see the examples below.
> > > +		 The first one controls the devices decoding window and
> > > +		 the second one controls the SDRAM decoding window.
> > > +
> > > +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> > > +                 the second cell for the address offset within the window.
> > > +
> > > +- size-cells:    Must be '1'.
> > > +
> > > +- ranges:        Must be set up to provide a proper translation for each child.
> > > +	         See the examples below.
> > 
> > You should explain here what the policy is regarding windows set up by the
> > boot loader. Are the ranges in here required to be the ones that the boot
> > loader has preconfigured, or are they the ones that the mbus driver is supposed
> > to set up?
> > 
> 
> Actually, I might be confused but the kernel MBus driver is completely
> independent from the bootloader's -except in the internal register case,
> (because we don't touch that from where the bootloader left it).

Right, then state that clearly in the binding.

> What happens is that any decoding window that was setup by the bootloader,
> is wiped and completely new windows are allocated using the translations
> in the DT, as described by this binding.
> 
> This was the case from the start with the old MBus driver. FWIW, I think
> it's actually the best choice that can be made: it makes the kernel
> independent of the previous setting.
> 
> I know you've suggested differently in the past, but I'm not sure I
> understand what's the benefit in keeping the bootloaders configuration.

The device tree /normally/ describes things that are either wired up
in hardware or set up by the boot loader. Describing things that the
boot loader may or may not have set up and that the kernel should
set up but may ignore if it wants to is a bit fishy, but it seems
that you have decided to do it that way. You should definitely
document the fact that all ranges except the "internal-regs" are just
suggestions and cannot be relied on to be present at boot time.

A cleaner approach would be to require that all ranges here are exactly
the ones that have been configured by the boot loader, and state the
OS can either assume that they are valid or can reprogram them as
needed. That would imply that you don't actually need an mbus driver
unless you want to dynamically reassign the windows.

Please also add a property that describes the address range in which
the windows might be reassigned, i.e. everything that is not RAM.

> > > +Each child device needs at least a 'ranges' property. If the child is avaiable
> > > +(i.e. status not 'disabled'), then the MBus driver creates a decoding window
> > > +for it. For instance, in the example below the BootROM child is specified:
> > > +
> > > +	soc {
> > > +		compatible = "marvell,armada370-mbus", "simple-bus";
> > > +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> > > +		#address-cells = <2>;
> > > +		#size-cells = <1>;
> > > +
> > > +		ranges = < ... /* other entries */
> > > +			   0x011d0000 0 0 0xfff00000 0x100000>;
> > > +
> > > +		bootrom {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			ranges = <0 0x011d0000 0 0x100000>;
> > > +		};
> > > +
> > > +		/* other children */
> > > +		...
> > > +	};
> > 
> > Do you really want to require the child to provide a "ranges" property?
> > I think this makes it more complicated to specify devices that belong
> > into the "internal-regs" category.
> > 
> 
> No, this text is actually a left-over from the previous patchset, in
> current v3 patchset MBus children are *not* required to have any ranges.
> On the otherside, although you will need one except in the most trivial
> cases like for the BootROM.

Ok.

	Arnd

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Hi Arnd,

On Tue, Jun 18, 2013 at 11:45:26PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
> > > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > > +Required properties:
> > > > +
> > > > +- compatible:	 Should be set to one of the following:
> > > > +		 marvell,armada370-mbus
> > > > +		 marvell,armadaxp-mbus
> > > > +
> > > > +- reg:		 Device's register space.
> > > > +		 Two entries are expected, see the examples below.
> > > > +		 The first one controls the devices decoding window and
> > > > +		 the second one controls the SDRAM decoding window.
> > > > +
> > > > +- address-cells: Must be '2'. The first cell for the MBus ID encoding,
> > > > +                 the second cell for the address offset within the window.
> > > > +
> > > > +- size-cells:    Must be '1'.
> > > > +
> > > > +- ranges:        Must be set up to provide a proper translation for each child.
> > > > +	         See the examples below.
> > > 
> > > You should explain here what the policy is regarding windows set up by the
> > > boot loader. Are the ranges in here required to be the ones that the boot
> > > loader has preconfigured, or are they the ones that the mbus driver is supposed
> > > to set up?
> > > 
> > 
> > Actually, I might be confused but the kernel MBus driver is completely
> > independent from the bootloader's -except in the internal register case,
> > (because we don't touch that from where the bootloader left it).
> 
> Right, then state that clearly in the binding.
> 
> > What happens is that any decoding window that was setup by the bootloader,
> > is wiped and completely new windows are allocated using the translations
> > in the DT, as described by this binding.
> > 
> > This was the case from the start with the old MBus driver. FWIW, I think
> > it's actually the best choice that can be made: it makes the kernel
> > independent of the previous setting.
> > 
> > I know you've suggested differently in the past, but I'm not sure I
> > understand what's the benefit in keeping the bootloaders configuration.
> 
> The device tree /normally/ describes things that are either wired up
> in hardware or set up by the boot loader. Describing things that the
> boot loader may or may not have set up and that the kernel should
> set up but may ignore if it wants to is a bit fishy, but it seems
> that you have decided to do it that way. You should definitely
> document the fact that all ranges except the "internal-regs" are just
> suggestions and cannot be relied on to be present at boot time.
> 

Hold on! I've just noticed this, and I want to clarify something, just
to avoid mis-interpretations. The binding is not saying "the windows
described through this ranges are present at boot time".

Rather, it is "this binding will guarantee that the windows described 
in it will be present after the mbus allocates them".

Does it sound too fishy?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> > > What happens is that any decoding window that was setup by the bootloader,
> > > is wiped and completely new windows are allocated using the translations
> > > in the DT, as described by this binding.
> > > 
> > > This was the case from the start with the old MBus driver. FWIW, I think
> > > it's actually the best choice that can be made: it makes the kernel
> > > independent of the previous setting.
> > > 
> > > I know you've suggested differently in the past, but I'm not sure I
> > > understand what's the benefit in keeping the bootloaders configuration.
> > 
> > The device tree normally describes things that are either wired up
> > in hardware or set up by the boot loader. Describing things that the
> > boot loader may or may not have set up and that the kernel should
> > set up but may ignore if it wants to is a bit fishy, but it seems
> > that you have decided to do it that way. You should definitely
> > document the fact that all ranges except the "internal-regs" are just
> > suggestions and cannot be relied on to be present at boot time.
> > 
> 
> Hold on! I've just noticed this, and I want to clarify something, just
> to avoid mis-interpretations. The binding is not saying "the windows
> described through this ranges are present at boot time".
> 
> Rather, it is "this binding will guarantee that the windows described 
> in it will be present after the mbus allocates them".
> 
> Does it sound too fishy?

I don't think it's a guarantee that the binding can make. The binding
describes the interface between the hardware/firmware and the kernel,
not an interface between one kernel driver and another.

You could instead write:

"The ranges property defines a set of mbus windows that are expected
to be set by the operating system and that are guaranteed to be free
of overlaps with one another or with the system memory ranges.
Each entry in the property refers to exactly one window. If an
operating system choses to use a different set of mbus windows,
it must ensure that any address translations performed from downstream
devices are adapted accordingly. The operating system may insert
additional mbus windows that do not conflict with the ones listed
in the ranges, e.g. for mapping PCIe devices. As a special case,
the internal register window must be set up by the boot loader
at the address listed in the ranges property, since the operating
uses it to set up the other windows."

	Arnd

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

On Wed, Jun 19, 2013 at 09:08:30PM +0200, Arnd Bergmann wrote:
> On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> > > > What happens is that any decoding window that was setup by the bootloader,
> > > > is wiped and completely new windows are allocated using the translations
> > > > in the DT, as described by this binding.
> > > > 
> > > > This was the case from the start with the old MBus driver. FWIW, I think
> > > > it's actually the best choice that can be made: it makes the kernel
> > > > independent of the previous setting.
> > > > 
> > > > I know you've suggested differently in the past, but I'm not sure I
> > > > understand what's the benefit in keeping the bootloaders configuration.
> > > 
> > > The device tree normally describes things that are either wired up
> > > in hardware or set up by the boot loader. Describing things that the
> > > boot loader may or may not have set up and that the kernel should
> > > set up but may ignore if it wants to is a bit fishy, but it seems
> > > that you have decided to do it that way. You should definitely
> > > document the fact that all ranges except the "internal-regs" are just
> > > suggestions and cannot be relied on to be present at boot time.
> > > 
> > 
> > Hold on! I've just noticed this, and I want to clarify something, just
> > to avoid mis-interpretations. The binding is not saying "the windows
> > described through this ranges are present at boot time".
> > 
> > Rather, it is "this binding will guarantee that the windows described 
> > in it will be present after the mbus allocates them".
> > 
> > Does it sound too fishy?
> 
> I don't think it's a guarantee that the binding can make. The binding
> describes the interface between the hardware/firmware and the kernel,
> not an interface between one kernel driver and another.
> 
> You could instead write:
> 
> "The ranges property defines a set of mbus windows that are expected
> to be set by the operating system and that are guaranteed to be free
> of overlaps with one another or with the system memory ranges.
> Each entry in the property refers to exactly one window. If an
> operating system choses to use a different set of mbus windows,
> it must ensure that any address translations performed from downstream
> devices are adapted accordingly. The operating system may insert
> additional mbus windows that do not conflict with the ones listed
> in the ranges, e.g. for mapping PCIe devices. As a special case,
> the internal register window must be set up by the boot loader
> at the address listed in the ranges property, since the operating
> uses it to set up the other windows."
> 

Nice!

Shamelessly copy-pasted into the binding documentation.

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Cooper]

On Wed, Jun 19, 2013 at 04:29:16PM -0300, Ezequiel Garcia wrote:
> On Wed, Jun 19, 2013 at 09:08:30PM +0200, Arnd Bergmann wrote:
> > On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> > > > > What happens is that any decoding window that was setup by the bootloader,
> > > > > is wiped and completely new windows are allocated using the translations
> > > > > in the DT, as described by this binding.
> > > > > 
> > > > > This was the case from the start with the old MBus driver. FWIW, I think
> > > > > it's actually the best choice that can be made: it makes the kernel
> > > > > independent of the previous setting.
> > > > > 
> > > > > I know you've suggested differently in the past, but I'm not sure I
> > > > > understand what's the benefit in keeping the bootloaders configuration.
> > > > 
> > > > The device tree normally describes things that are either wired up
> > > > in hardware or set up by the boot loader. Describing things that the
> > > > boot loader may or may not have set up and that the kernel should
> > > > set up but may ignore if it wants to is a bit fishy, but it seems
> > > > that you have decided to do it that way. You should definitely
> > > > document the fact that all ranges except the "internal-regs" are just
> > > > suggestions and cannot be relied on to be present at boot time.
> > > > 
> > > 
> > > Hold on! I've just noticed this, and I want to clarify something, just
> > > to avoid mis-interpretations. The binding is not saying "the windows
> > > described through this ranges are present at boot time".
> > > 
> > > Rather, it is "this binding will guarantee that the windows described 
> > > in it will be present after the mbus allocates them".
> > > 
> > > Does it sound too fishy?
> > 
> > I don't think it's a guarantee that the binding can make. The binding
> > describes the interface between the hardware/firmware and the kernel,
> > not an interface between one kernel driver and another.
> > 
> > You could instead write:
> > 
> > "The ranges property defines a set of mbus windows that are expected
> > to be set by the operating system and that are guaranteed to be free
> > of overlaps with one another or with the system memory ranges.
> > Each entry in the property refers to exactly one window. If an
> > operating system choses to use a different set of mbus windows,
> > it must ensure that any address translations performed from downstream
> > devices are adapted accordingly. The operating system may insert
> > additional mbus windows that do not conflict with the ones listed
> > in the ranges, e.g. for mapping PCIe devices. As a special case,
> > the internal register window must be set up by the boot loader
> > at the address listed in the ranges property, since the operating

...system...

> > uses it to set up the other windows."
> > 
> 
> Nice!
> 
> Shamelessly copy-pasted into the binding documentation.

thx,

Jason.

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote:
> +
> +IIAA0000
> +
> +Where:
> +  -- I = Marvell defined target ID for programmable windows
> +  -- A = Marvell defined target attributes for programmable windows

I thought we agreed to something like:

SIAA0000

Where 'S' is the designator for the special items like PCI-E and
internal-regs. 0 = normal target ids, 0xF = special ids.

The target is only 4 bits, the attr is 8, so a little doc update to
clarify this should be enough, no need to change the DTs.

Jason

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Sebastian Hesselbarth]

On 06/18/2013 07:46 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote:
>> +
>> +IIAA0000
>> +
>> +Where:
>> +  -- I = Marvell defined target ID for programmable windows
>> +  -- A = Marvell defined target attributes for programmable windows
>
> I thought we agreed to something like:
>
> SIAA0000
>
> Where 'S' is the designator for the special items like PCI-E and
> internal-regs. 0 = normal target ids, 0xF = special ids.
>
> The target is only 4 bits, the attr is 8, so a little doc update to
> clarify this should be enough, no need to change the DTs.

+1 for SIAA0000, as it allows to use MBUS_ID also for those fake
windows. It makes it more readable IMHO.

Also allows you to have up to 40b offset, which _might_ be important
with LPAE enabled.

Sebastian

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 18 June 2013, Sebastian Hesselbarth wrote:
> On 06/18/2013 07:46 PM, Jason Gunthorpe wrote:
> > On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote:
> >> +
> >> +IIAA0000
> >> +
> >> +Where:
> >> +  -- I = Marvell defined target ID for programmable windows
> >> +  -- A = Marvell defined target attributes for programmable windows
> >
> > I thought we agreed to something like:
> >
> > SIAA0000
> >
> > Where 'S' is the designator for the special items like PCI-E and
> > internal-regs. 0 = normal target ids, 0xF = special ids.
> >
> > The target is only 4 bits, the attr is 8, so a little doc update to
> > clarify this should be enough, no need to change the DTs.
> 
> +1 for SIAA0000, as it allows to use MBUS_ID also for those fake
> windows. It makes it more readable IMHO.

+1

> Also allows you to have up to 40b offset, which might be important
> with LPAE enabled.

Not with the current generation I think, since the mbus windows are
32 bit only, but it would avoid having to come up with a new format
for a potential future-generation mbus that has wider address.

	Arnd

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Sebastian Hesselbarth]

On 06/18/2013 08:39 PM, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Sebastian Hesselbarth wrote:
>> Also allows you to have up to 40b offset, which might be important
>> with LPAE enabled.
>
> Not with the current generation I think, since the mbus windows are
> 32 bit only, but it would avoid having to come up with a new format
> for a potential future-generation mbus that has wider address.

Yeah, I also recall Thomas or Gregory mention a 32b limitation in
remap windows. But we don't need to waste addresses here

And even if SIAA0000 is a concern because there may be target id >15
someday, we can still have IIAASS00 instead of IIAA00SS. I guess
LPAE will not rise above 40b quickly ;)

Sebastian

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 08:44:30PM +0200, Sebastian Hesselbarth wrote:
> On 06/18/2013 08:39 PM, Arnd Bergmann wrote:
> >On Tuesday 18 June 2013, Sebastian Hesselbarth wrote:
> >>Also allows you to have up to 40b offset, which might be important
> >>with LPAE enabled.
> >
> >Not with the current generation I think, since the mbus windows are
> >32 bit only, but it would avoid having to come up with a new format
> >for a potential future-generation mbus that has wider address.
> 
> Yeah, I also recall Thomas or Gregory mention a 32b limitation in
> remap windows. But we don't need to waste addresses here
> 
> And even if SIAA0000 is a concern because there may be target id >15
> someday, we can still have IIAASS00 instead of IIAA00SS. I guess
> LPAE will not rise above 40b quickly ;)

S = 0 means 4 bit I, 8 bit A
S = F means special
S = 1 could mean 16 bit I, etc , etc

Yes, we could define things as 'SIAAoooo oooooooo'

But remember 'o' is the offset within the window, it is not related to
LPAE.

To need > 32 bits 'o' you need to have windows > 4G in size. The only
thing that could possibly do that is PCI-E, and it is all special
anyhow..

The mbus top level ranges remap already supports >4G locations for
the windows, even though current hardware cannot do that.

Jason

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Sebastian Hesselbarth]

On 06/18/2013 08:47 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:44:30PM +0200, Sebastian Hesselbarth wrote:
>> Yeah, I also recall Thomas or Gregory mention a 32b limitation in
>> remap windows. But we don't need to waste addresses here
>>
>> And even if SIAA0000 is a concern because there may be target id>15
>> someday, we can still have IIAASS00 instead of IIAA00SS. I guess
>> LPAE will not rise above 40b quickly ;)
>
> S = 0 means 4 bit I, 8 bit A
> S = F means special
> S = 1 could mean 16 bit I, etc , etc

S & 0x8 == 0x0 means 7b target
S & 0x8 == 0x8 means 7b special ?

> Yes, we could define things as 'SIAAoooo oooooooo'
>
> But remember 'o' is the offset within the window, it is not related to
> LPAE.
>
> To need>  32 bits 'o' you need to have windows>  4G in size. The only
> thing that could possibly do that is PCI-E, and it is all special
> anyhow..
>
> The mbus top level ranges remap already supports>4G locations for
> the windows, even though current hardware cannot do that.

True. But as Arnd also mentioned, I don't think it will ever be a
problem at all. Possibly there will never be any future SoC with mbus
that will either allow >32b remap base addresses nor >4G size.

But never the less, IIAA00SS (which is used in v3 of the patches) will
limit both to 32b/4G forever.

And we already have +3 for SIAAoooo ;)

Sebastian

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 08:59:03PM +0200, Sebastian Hesselbarth wrote:

> >S = 0 means 4 bit I, 8 bit A
> >S = F means special
> >S = 1 could mean 16 bit I, etc , etc
> 
> S & 0x8 == 0x0 means 7b target
> S & 0x8 == 0x8 means 7b special ?

No need, special == mbus driver defined. There is no target ID.

The forms could be:

 0IAA0000
 FK000000
   - K=0 -> internal regs
   - K=1 -> PCI-E thingy
    etc
 1IIAA000 (future expansion example)


> >The mbus top level ranges remap already supports>4G locations for
> >the windows, even though current hardware cannot do that.
> 
> True. But as Arnd also mentioned, I don't think it will ever be a
> problem at all. Possibly there will never be any future SoC with mbus
> that will either allow >32b remap base addresses nor >4G size.

Actually, I think the failure to allow > 32b remap is a mistake
on Marvell's part. Linux needs as much low memory as possible, moving
things above 4G gives you more low SDRAM.

So I'd hope to see 40bit addressing for MBUS windows in a future chip.

But again, that already works with what Ezequiel has..

To be very clear, the only issue with the 32 bit offset is if we need
to describe an offset into a target in DT that is larger than 32
bits. The only use of offsets in DT is for internal regs. The
need for a > 32 bit offset in DT does not exist with the current
architecture.

Jason

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Sebastian Hesselbarth]

On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:59:03PM +0200, Sebastian Hesselbarth wrote:
>
>>> S = 0 means 4 bit I, 8 bit A
>>> S = F means special
>>> S = 1 could mean 16 bit I, etc , etc
>>
>> S&  0x8 == 0x0 means 7b target
>> S&  0x8 == 0x8 means 7b special ?
>
> No need, special == mbus driver defined. There is no target ID.
>
> The forms could be:
>
>   0IAA0000
>   FK000000
>     - K=0 ->  internal regs
>     - K=1 ->  PCI-E thingy
>      etc
>   1IIAA000 (future expansion example)

Ok, got it. Any encoding is fine that allows to distinguish real
remap windows and fake ones. I assumed that maybe someday there
will be more than 4b target id so 0x80 as special case indicator
leaves 7b of normal target id in the _current_ mapping.

But yes, we can always invent a new encoding compatible with the
current mapping to allow more bits for either target id or attr
encoding.

I am fine with anything that leaves some room for >32b remap
windows.

>>> The mbus top level ranges remap already supports>4G locations for
>>> the windows, even though current hardware cannot do that.
>>
>> True. But as Arnd also mentioned, I don't think it will ever be a
>> problem at all. Possibly there will never be any future SoC with mbus
>> that will either allow>32b remap base addresses nor>4G size.
>
> Actually, I think the failure to allow>  32b remap is a mistake
> on Marvell's part. Linux needs as much low memory as possible, moving
> things above 4G gives you more low SDRAM.
>
> So I'd hope to see 40bit addressing for MBUS windows in a future chip.
>
> But again, that already works with what Ezequiel has..

Yeah, but currently Ezequiel e.g. uses 0xffff0001 for internal regs
that will not look nice with MBUS_ID(0xff,0xff) | 0x0001.

The whole point in the last mails was to ensure, Ezequiel will update
all remap windows to use MBUS_ID() and the fake ones to
MBUS_ID(0xf0,0x01), MBUS_ID(0xf0,0x02), aso.

Otherwise, I agree on _everything_ you said! :)

> To be very clear, the only issue with the 32 bit offset is if we need
> to describe an offset into a target in DT that is larger than 32
> bits. The only use of offsets in DT is for internal regs. The
> need for a>  32 bit offset in DT does not exist with the current
> architecture.
>
> Jason

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Hi Sebastian,

You loose +1 internet points for dropping me from Cc ;-)

On Tue, Jun 18, 2013 at 09:27:28PM +0200, Sebastian Hesselbarth wrote:
> On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> >
> > The forms could be:
> >
> >   0IAA0000
> >   FK000000
> >     - K=0 ->  internal regs
> >     - K=1 ->  PCI-E thingy
> >      etc
> >   1IIAA000 (future expansion example)
> 
> Ok, got it. Any encoding is fine that allows to distinguish real
> remap windows and fake ones. I assumed that maybe someday there
> will be more than 4b target id so 0x80 as special case indicator
> leaves 7b of normal target id in the _current_ mapping.
> 

I'm also wondering about why we not care about target IDs being more
than 4 bits.

Jason: (I'm checking now but perhaps you know better than me):
Is there any MBus-architectural reason for you assuring the
target ID will always be within 4-bits?

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 05:49:11PM -0300, Ezequiel Garcia wrote:
> Hi Sebastian,
> 
> You loose +1 internet points for dropping me from Cc ;-)
> 
> On Tue, Jun 18, 2013 at 09:27:28PM +0200, Sebastian Hesselbarth wrote:
> > On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> > >
> > > The forms could be:
> > >
> > >   0IAA0000
> > >   FK000000
> > >     - K=0 ->  internal regs
> > >     - K=1 ->  PCI-E thingy
> > >      etc
> > >   1IIAA000 (future expansion example)
> > 
> > Ok, got it. Any encoding is fine that allows to distinguish real
> > remap windows and fake ones. I assumed that maybe someday there
> > will be more than 4b target id so 0x80 as special case indicator
> > leaves 7b of normal target id in the _current_ mapping.
> > 
> 
> I'm also wondering about why we not care about target IDs being more
> than 4 bits.
> 
> Jason: (I'm checking now but perhaps you know better than me):
> Is there any MBus-architectural reason for you assuring the
> target ID will always be within 4-bits?

The manuals I have for the register set say 4 bits is allocated for
mbus targets.. Are yours different?

If they change it then the window register layout changes and then the
mbus driver probably need to change as well.

Jason

[PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 02:55:22PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 05:49:11PM -0300, Ezequiel Garcia wrote:
> > Hi Sebastian,
> > 
> > You loose +1 internet points for dropping me from Cc ;-)
> > 
> > On Tue, Jun 18, 2013 at 09:27:28PM +0200, Sebastian Hesselbarth wrote:
> > > On 06/18/2013 09:10 PM, Jason Gunthorpe wrote:
> > > >
> > > > The forms could be:
> > > >
> > > >   0IAA0000
> > > >   FK000000
> > > >     - K=0 ->  internal regs
> > > >     - K=1 ->  PCI-E thingy
> > > >      etc
> > > >   1IIAA000 (future expansion example)
> > > 
> > > Ok, got it. Any encoding is fine that allows to distinguish real
> > > remap windows and fake ones. I assumed that maybe someday there
> > > will be more than 4b target id so 0x80 as special case indicator
> > > leaves 7b of normal target id in the _current_ mapping.
> > > 
> > 
> > I'm also wondering about why we not care about target IDs being more
> > than 4 bits.
> > 
> > Jason: (I'm checking now but perhaps you know better than me):
> > Is there any MBus-architectural reason for you assuring the
> > target ID will always be within 4-bits?
> 
> The manuals I have for the register set say 4 bits is allocated for
> mbus targets.. Are yours different?
> 

Nope. 4 bits here as well.

> If they change it then the window register layout changes and then the
> mbus driver probably need to change as well.
> 

Okey, then.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 04/12] ARM: mvebu: Initialize MBus using the DT binding

[Ezequiel Garcia]

Now that the mbus device tree binding has been introduced, we can
switch over to it.

Also, and since the initialization of the mbus driver is quite
fundamental for the system to work properly, this patch adds a BUG()
in case mbus fails to initialize.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c | 34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 0dbc370..4c312be 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -34,44 +34,12 @@ static void __init armada_370_xp_map_io(void)
 	debug_ll_io_init();
 }
 
-/*
- * This initialization will be replaced by a DT-based
- * initialization once the mvebu-mbus driver gains DT support.
- */
-
-#define ARMADA_370_XP_MBUS_WINS_OFFS   0x20000
-#define ARMADA_370_XP_MBUS_WINS_SIZE   0x100
-#define ARMADA_370_XP_SDRAM_WINS_OFFS  0x20180
-#define ARMADA_370_XP_SDRAM_WINS_SIZE  0x20
-
-static void __init armada_370_xp_mbus_init(void)
-{
-	char *mbus_soc_name;
-	struct device_node *dn;
-	const __be32 mbus_wins_offs = cpu_to_be32(ARMADA_370_XP_MBUS_WINS_OFFS);
-	const __be32 sdram_wins_offs = cpu_to_be32(ARMADA_370_XP_SDRAM_WINS_OFFS);
-
-	if (of_machine_is_compatible("marvell,armada370"))
-		mbus_soc_name = "marvell,armada370-mbus";
-	else
-		mbus_soc_name = "marvell,armadaxp-mbus";
-
-	dn = of_find_node_by_name(NULL, "internal-regs");
-	BUG_ON(!dn);
-
-	mvebu_mbus_init(mbus_soc_name,
-			of_translate_address(dn, &mbus_wins_offs),
-			ARMADA_370_XP_MBUS_WINS_SIZE,
-			of_translate_address(dn, &sdram_wins_offs),
-			ARMADA_370_XP_SDRAM_WINS_SIZE);
-}
-
 static void __init armada_370_xp_timer_and_clk_init(void)
 {
 	mvebu_clocks_init();
 	armada_370_xp_timer_init();
 	coherency_init();
-	armada_370_xp_mbus_init();
+	BUG_ON(mvebu_mbus_dt_init());
 #ifdef CONFIG_CACHE_L2X0
 	l2x0_of_init(0, ~0UL);
 #endif
-- 
1.8.1.5

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

The address decoding window to access the BootROM should not be
allocated programatically, but instead declared in the device tree.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-mvebu/platsmp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index 93f2f3a..d419fac 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -118,7 +118,6 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 	set_secondary_cpus_clock();
 	flush_cache_all();
 	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
-	mvebu_mbus_add_window("bootrom", 0xfff00000, SZ_1M);
 }
 
 struct smp_operations armada_xp_smp_ops __initdata = {
-- 
1.8.1.5

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 08:25:30AM -0300, Ezequiel Garcia wrote:
> The address decoding window to access the BootROM should not be
> allocated programatically, but instead declared in the device tree.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>  arch/arm/mach-mvebu/platsmp.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
> index 93f2f3a..d419fac 100644
> +++ b/arch/arm/mach-mvebu/platsmp.c
> @@ -118,7 +118,6 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
>  	set_secondary_cpus_clock();
>  	flush_cache_all();
>  	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
> -	mvebu_mbus_add_window("bootrom", 0xfff00000, SZ_1M);
>  }

I think some kind of test is needed here. As I understand it the SMP
startup uses a trampoline in the boot rom and the boot rom *must* be
mapped to 0xfff00000 ?

Verifying the DT is setup this way and aborting if it is not seems
like a good idea..

Jason

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 11:39:06AM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:25:30AM -0300, Ezequiel Garcia wrote:
> > The address decoding window to access the BootROM should not be
> > allocated programatically, but instead declared in the device tree.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >  arch/arm/mach-mvebu/platsmp.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
> > index 93f2f3a..d419fac 100644
> > +++ b/arch/arm/mach-mvebu/platsmp.c
> > @@ -118,7 +118,6 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
> >  	set_secondary_cpus_clock();
> >  	flush_cache_all();
> >  	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
> > -	mvebu_mbus_add_window("bootrom", 0xfff00000, SZ_1M);
> >  }
> 
> I think some kind of test is needed here. As I understand it the SMP
> startup uses a trampoline in the boot rom and the boot rom *must* be
> mapped to 0xfff00000 ?
>

Yes, that's my understanding as well, but I will do some testing since
it should be interesting...

> Verifying the DT is setup this way and aborting if it is not seems
> like a good idea..
> 

I have no problem doing that, but to me it sounds as it's the
responsability of the one that writes the DT, no?

Maybe this is a requirement for this SoC, but not for another...
so, why should the kernel *check* for that?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 04:43:31PM -0300, Ezequiel Garcia wrote:

> > I think some kind of test is needed here. As I understand it the SMP
> > startup uses a trampoline in the boot rom and the boot rom *must* be
> > mapped to 0xfff00000 ?

> Yes, that's my understanding as well, but I will do some testing since
> it should be interesting...

If it is like the earlier chips you will also have the choice of
something based at 0, there is a register bit that sets the reset
address.
 
> > Verifying the DT is setup this way and aborting if it is not seems
> > like a good idea..

> I have no problem doing that, but to me it sounds as it's the
> responsability of the one that writes the DT, no?

Having the kernel enforce that the DT node is present and at the right
location, I think, is helpful for the bootloader folks to ensure they
write correct DTs.

> Maybe this is a requirement for this SoC, but not for another...
> so, why should the kernel *check* for that?

The function was called armada_xp_smp_prepare_cpus - all Armada XP's
will work like this..

Jason

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 01:51:11PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 04:43:31PM -0300, Ezequiel Garcia wrote:
> 
> > > I think some kind of test is needed here. As I understand it the SMP
> > > startup uses a trampoline in the boot rom and the boot rom *must* be
> > > mapped to 0xfff00000 ?
> 
> > Yes, that's my understanding as well, but I will do some testing since
> > it should be interesting...
> 
> If it is like the earlier chips you will also have the choice of
> something based at 0, there is a register bit that sets the reset
> address.
>  
> > > Verifying the DT is setup this way and aborting if it is not seems
> > > like a good idea..
> 
> > I have no problem doing that, but to me it sounds as it's the
> > responsability of the one that writes the DT, no?
> 
> Having the kernel enforce that the DT node is present and at the right
> location, I think, is helpful for the bootloader folks to ensure they
> write correct DTs.
>

Granted. But then I wonder... why do we bother to put the BootROM in the
DT window if we're going to check for a fixed address it in any case?

> > Maybe this is a requirement for this SoC, but not for another...
> > so, why should the kernel *check* for that?
> 
> The function was called armada_xp_smp_prepare_cpus - all Armada XP's
> will work like this..
> 

Right. Silly objection.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 05:02:42PM -0300, Ezequiel Garcia wrote:
> > Having the kernel enforce that the DT node is present and at the right
> > location, I think, is helpful for the bootloader folks to ensure they
> > write correct DTs.

> Granted. But then I wonder... why do we bother to put the BootROM in the
> DT window if we're going to check for a fixed address it in any case?

Code re-use in the mbus driver?

Maybe future SOCs in this family will have programmable SMP startup
addresses?

Non-SMP systems don't need to map the boot rom at all?

Jason

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 02:10:21PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 05:02:42PM -0300, Ezequiel Garcia wrote:
> > > Having the kernel enforce that the DT node is present and at the right
> > > location, I think, is helpful for the bootloader folks to ensure they
> > > write correct DTs.
> 
> > Granted. But then I wonder... why do we bother to put the BootROM in the
> > DT window if we're going to check for a fixed address it in any case?
> 
> Code re-use in the mbus driver?
> 
> Maybe future SOCs in this family will have programmable SMP startup
> addresses?
> 
> Non-SMP systems don't need to map the boot rom at all?
> 

Three great answers :)

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 11:39:06AM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:25:30AM -0300, Ezequiel Garcia wrote:
> > The address decoding window to access the BootROM should not be
> > allocated programatically, but instead declared in the device tree.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >  arch/arm/mach-mvebu/platsmp.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
> > index 93f2f3a..d419fac 100644
> > +++ b/arch/arm/mach-mvebu/platsmp.c
> > @@ -118,7 +118,6 @@ void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
> >  	set_secondary_cpus_clock();
> >  	flush_cache_all();
> >  	set_cpu_coherent(cpu_logical_map(smp_processor_id()), 0);
> > -	mvebu_mbus_add_window("bootrom", 0xfff00000, SZ_1M);
> >  }
> 
> I think some kind of test is needed here. As I understand it the SMP
> startup uses a trampoline in the boot rom and the boot rom *must* be
> mapped to 0xfff00000 ?
> 

Indeed, setting up 0xfff00000 is a *must*.

> Verifying the DT is setup this way and aborting if it is not seems
> like a good idea..
> 

I agree it's a nice idea, but I'm not too sure how to accomplish this
in a simple and generic way. There's nothing in the DT that allows you
to know which of the ranges entries correspond to the BootROM, unless you go
through each of the entries comparing against the known target ID and
attribute.

You could also do a 'of_find_node_by_name(NULL, "bootrom");' but the binding
no longer requires to even have any children for the window.

Any ideas?

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Jason Gunthorpe]

On Wed, Jun 19, 2013 at 07:02:00AM -0300, Ezequiel Garcia wrote:

> > Verifying the DT is setup this way and aborting if it is not seems
> > like a good idea..
> 
> I agree it's a nice idea, but I'm not too sure how to accomplish this
> in a simple and generic way. There's nothing in the DT that allows you
> to know which of the ranges entries correspond to the BootROM, unless you go
> through each of the entries comparing against the known target ID and
> attribute.

I think you need to have a defined compatible string for the bootrom,
use one of the of_find.. functions to locate the node, then translate
the regs to get a CPU address, ensure it is the right base and size..

> You could also do a 'of_find_node_by_name(NULL, "bootrom");' but the
> binding no longer requires to even have any children for the window.

You'll need to define a bootrom binding. There is not a lot of sense
in including things in the DT if the kernel can't find them and use
them.

Jason

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

On Wed, Jun 19, 2013 at 10:58:34AM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 19, 2013 at 07:02:00AM -0300, Ezequiel Garcia wrote:
> 
> > > Verifying the DT is setup this way and aborting if it is not seems
> > > like a good idea..
> > 
> > I agree it's a nice idea, but I'm not too sure how to accomplish this
> > in a simple and generic way. There's nothing in the DT that allows you
> > to know which of the ranges entries correspond to the BootROM, unless you go
> > through each of the entries comparing against the known target ID and
> > attribute.
> 
> I think you need to have a defined compatible string for the bootrom,
> use one of the of_find.. functions to locate the node, then translate
> the regs to get a CPU address, ensure it is the right base and size..
> 

I wasn't sure you wanted to panic(), to clip on available CPUs,
or to just do a pr_warn / WARN(), so here's a piece of code:
(disclaimer: non-tested, non-compiled, etc.)

	/*
	 * In order to boot the secondary CPUs we need to ensure
	 * the bootROM is mapped at the correct address.
	 */
	node = of_find_compatible_node(NULL, NULL, "bootrom");
	if (!node) {
		pr_warn("No 'bootrom' node found");
		return;
	}

	err = of_address_to_resource(node, 0, &res);
	if (err < 0) {
		pr_warn("Cannot get 'bootrom' node address");
		return;
	}

	if (res.start != AXP_BOOTROM_BASE||
	    resource_size(&res) != AXP_BOOTROM_SIZE) {
		pr_warn("bootrom address is incorrect");
		return;
	}

How does it look?

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Jason Gunthorpe]

On Wed, Jun 19, 2013 at 02:58:24PM -0300, Ezequiel Garcia wrote:

> I wasn't sure you wanted to panic(), to clip on available CPUs,
> or to just do a pr_warn / WARN(), so here's a piece of code:
> (disclaimer: non-tested, non-compiled, etc.)

Up to you, but you know it won't work if it isn't right so continuing
on to try and startup the CPUs is not ideal. panic is probably
reasonable?
 
> 	node = of_find_compatible_node(NULL, NULL, "bootrom");

"marvell,bootrom" ?

Jason

[PATCH v3 05/12] ARM: mvebu: Remove the harcoded BootROM window allocation

[Ezequiel Garcia]

On Wed, Jun 19, 2013 at 12:03:20PM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 19, 2013 at 02:58:24PM -0300, Ezequiel Garcia wrote:
> 
> > I wasn't sure you wanted to panic(), to clip on available CPUs,
> > or to just do a pr_warn / WARN(), so here's a piece of code:
> > (disclaimer: non-tested, non-compiled, etc.)
> 
> Up to you, but you know it won't work if it isn't right so continuing
> on to try and startup the CPUs is not ideal. panic is probably
> reasonable?
>  

Well, the other CPUs don't start but the system itself does not die,
(or at least that's the case on my systems).

It's up to you, I'm fine with either, although I'm inclined to just
panic.

> > 	node = of_find_compatible_node(NULL, NULL, "bootrom");
> 
> "marvell,bootrom" ?
> 

Yes, indeed.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 06/12] memory: mvebu-devbus: Remove address decoding window workaround

[Ezequiel Garcia]

Now that mbus device tree binding has been introduced, remove the address
decoding window management from this driver.
A suitable 'ranges' entry should be added to the devbus-compatible node in
the device tree, as described by the mbus binding documentation.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
 1 file changed, 2 insertions(+), 62 deletions(-)

diff --git a/drivers/memory/mvebu-devbus.c b/drivers/memory/mvebu-devbus.c
index 978e8e3..94c9248 100644
--- a/drivers/memory/mvebu-devbus.c
+++ b/drivers/memory/mvebu-devbus.c
@@ -208,16 +208,11 @@ static int mvebu_devbus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
-	struct device_node *parent;
 	struct devbus *devbus;
 	struct resource *res;
 	struct clk *clk;
 	unsigned long rate;
-	const __be32 *ranges;
-	int err, cs;
-	int addr_cells, p_addr_cells, size_cells;
-	int ranges_len, tuple_len;
-	u32 base, size;
+	int err;
 
 	devbus = devm_kzalloc(&pdev->dev, sizeof(struct devbus), GFP_KERNEL);
 	if (!devbus)
@@ -248,68 +243,13 @@ static int mvebu_devbus_probe(struct platform_device *pdev)
 		return err;
 
 	/*
-	 * Allocate an address window for this device.
-	 * If the device probing fails, then we won't be able to
-	 * remove the allocated address decoding window.
-	 *
-	 * FIXME: This is only a temporary hack! We need to do this here
-	 * because we still don't have device tree bindings for mbus.
-	 * Once that support is added, we will declare these address windows
-	 * statically in the device tree, and remove the window configuration
-	 * from here.
-	 */
-
-	/*
-	 * Get the CS to choose the window string.
-	 * This is a bit hacky, but it will be removed once the
-	 * address windows are declared in the device tree.
-	 */
-	cs = (((unsigned long)devbus->base) % 0x400) / 8;
-
-	/*
-	 * Parse 'ranges' property to obtain a (base,size) window tuple.
-	 * This will be removed once the address windows
-	 * are declared in the device tree.
-	 */
-	parent = of_get_parent(node);
-	if (!parent)
-		return -EINVAL;
-
-	p_addr_cells = of_n_addr_cells(parent);
-	of_node_put(parent);
-
-	addr_cells = of_n_addr_cells(node);
-	size_cells = of_n_size_cells(node);
-	tuple_len = (p_addr_cells + addr_cells + size_cells) * sizeof(__be32);
-
-	ranges = of_get_property(node, "ranges", &ranges_len);
-	if (ranges == NULL || ranges_len != tuple_len)
-		return -EINVAL;
-
-	base = of_translate_address(node, ranges + addr_cells);
-	if (base == OF_BAD_ADDR)
-		return -EINVAL;
-	size = of_read_number(ranges + addr_cells + p_addr_cells, size_cells);
-
-	/*
-	 * Create an mbus address windows.
-	 * FIXME: Remove this, together with the above code, once the
-	 * address windows are declared in the device tree.
-	 */
-	err = mvebu_mbus_add_window(devbus_wins[cs], base, size);
-	if (err < 0)
-		return err;
-
-	/*
 	 * We need to create a child device explicitly from here to
 	 * guarantee that the child will be probed after the timing
 	 * parameters for the bus are written.
 	 */
 	err = of_platform_populate(node, NULL, NULL, dev);
-	if (err < 0) {
-		mvebu_mbus_del_window(base, size);
+	if (err < 0)
 		return err;
-	}
 
 	return 0;
 }
-- 
1.8.1.5

[PATCH v3 06/12] memory: mvebu-devbus: Remove address decoding window workaround

[Jason Cooper]

On Tue, Jun 18, 2013 at 08:25:31AM -0300, Ezequiel Garcia wrote:
> Now that mbus device tree binding has been introduced, remove the address
> decoding window management from this driver.
> A suitable 'ranges' entry should be added to the devbus-compatible node in
> the device tree, as described by the mbus binding documentation.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
>  1 file changed, 2 insertions(+), 62 deletions(-)

Unfortunately, the patch adding mvebu-devbus is going through gregkh's
tree.  Either this patch needs to wait for v3.12, or ask Greg if he can
take this one.

thx,

Jason.

> 
> diff --git a/drivers/memory/mvebu-devbus.c b/drivers/memory/mvebu-devbus.c
> index 978e8e3..94c9248 100644
> --- a/drivers/memory/mvebu-devbus.c
> +++ b/drivers/memory/mvebu-devbus.c
> @@ -208,16 +208,11 @@ static int mvebu_devbus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *node = pdev->dev.of_node;
> -	struct device_node *parent;
>  	struct devbus *devbus;
>  	struct resource *res;
>  	struct clk *clk;
>  	unsigned long rate;
> -	const __be32 *ranges;
> -	int err, cs;
> -	int addr_cells, p_addr_cells, size_cells;
> -	int ranges_len, tuple_len;
> -	u32 base, size;
> +	int err;
>  
>  	devbus = devm_kzalloc(&pdev->dev, sizeof(struct devbus), GFP_KERNEL);
>  	if (!devbus)
> @@ -248,68 +243,13 @@ static int mvebu_devbus_probe(struct platform_device *pdev)
>  		return err;
>  
>  	/*
> -	 * Allocate an address window for this device.
> -	 * If the device probing fails, then we won't be able to
> -	 * remove the allocated address decoding window.
> -	 *
> -	 * FIXME: This is only a temporary hack! We need to do this here
> -	 * because we still don't have device tree bindings for mbus.
> -	 * Once that support is added, we will declare these address windows
> -	 * statically in the device tree, and remove the window configuration
> -	 * from here.
> -	 */
> -
> -	/*
> -	 * Get the CS to choose the window string.
> -	 * This is a bit hacky, but it will be removed once the
> -	 * address windows are declared in the device tree.
> -	 */
> -	cs = (((unsigned long)devbus->base) % 0x400) / 8;
> -
> -	/*
> -	 * Parse 'ranges' property to obtain a (base,size) window tuple.
> -	 * This will be removed once the address windows
> -	 * are declared in the device tree.
> -	 */
> -	parent = of_get_parent(node);
> -	if (!parent)
> -		return -EINVAL;
> -
> -	p_addr_cells = of_n_addr_cells(parent);
> -	of_node_put(parent);
> -
> -	addr_cells = of_n_addr_cells(node);
> -	size_cells = of_n_size_cells(node);
> -	tuple_len = (p_addr_cells + addr_cells + size_cells) * sizeof(__be32);
> -
> -	ranges = of_get_property(node, "ranges", &ranges_len);
> -	if (ranges == NULL || ranges_len != tuple_len)
> -		return -EINVAL;
> -
> -	base = of_translate_address(node, ranges + addr_cells);
> -	if (base == OF_BAD_ADDR)
> -		return -EINVAL;
> -	size = of_read_number(ranges + addr_cells + p_addr_cells, size_cells);
> -
> -	/*
> -	 * Create an mbus address windows.
> -	 * FIXME: Remove this, together with the above code, once the
> -	 * address windows are declared in the device tree.
> -	 */
> -	err = mvebu_mbus_add_window(devbus_wins[cs], base, size);
> -	if (err < 0)
> -		return err;
> -
> -	/*
>  	 * We need to create a child device explicitly from here to
>  	 * guarantee that the child will be probed after the timing
>  	 * parameters for the bus are written.
>  	 */
>  	err = of_platform_populate(node, NULL, NULL, dev);
> -	if (err < 0) {
> -		mvebu_mbus_del_window(base, size);
> +	if (err < 0)
>  		return err;
> -	}
>  
>  	return 0;
>  }
> -- 
> 1.8.1.5
> 

[PATCH v3 06/12] memory: mvebu-devbus: Remove address decoding window workaround

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 18 Jun 2013 07:39:20 -0400, Jason Cooper wrote:
> On Tue, Jun 18, 2013 at 08:25:31AM -0300, Ezequiel Garcia wrote:
> > Now that mbus device tree binding has been introduced, remove the address
> > decoding window management from this driver.
> > A suitable 'ranges' entry should be added to the devbus-compatible node in
> > the device tree, as described by the mbus binding documentation.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
> >  1 file changed, 2 insertions(+), 62 deletions(-)
> 
> Unfortunately, the patch adding mvebu-devbus is going through gregkh's
> tree.  Either this patch needs to wait for v3.12, or ask Greg if he can
> take this one.

I believe that what we're looking for right now is a Acked-by from both
the Marvell maintainers, Arnd Bergmann and Jason Gunthorpe. Once we get
those Acked-by, we can figure out the merge strategy for this patch
set.

Thanks!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 06/12] memory: mvebu-devbus: Remove address decoding window workaround

[Jason Cooper]

On Tue, Jun 18, 2013 at 02:17:31PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 18 Jun 2013 07:39:20 -0400, Jason Cooper wrote:
> > On Tue, Jun 18, 2013 at 08:25:31AM -0300, Ezequiel Garcia wrote:
> > > Now that mbus device tree binding has been introduced, remove the address
> > > decoding window management from this driver.
> > > A suitable 'ranges' entry should be added to the devbus-compatible node in
> > > the device tree, as described by the mbus binding documentation.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
> > >  1 file changed, 2 insertions(+), 62 deletions(-)
> > 
> > Unfortunately, the patch adding mvebu-devbus is going through gregkh's
> > tree.  Either this patch needs to wait for v3.12, or ask Greg if he can
> > take this one.
> 
> I believe that what we're looking for right now is a Acked-by from both
> the Marvell maintainers, Arnd Bergmann and Jason Gunthorpe. Once we get
> those Acked-by, we can figure out the merge strategy for this patch
> set.

Right.  I was more looking to give Ezequiel a heads up because I think
he will have to extract devbus related changes out of the patches later
in this series.  I haven't had time to look closely yet, but wanted to
toss it out there so it didn't rear it's head later and throw things off
track.

thx,

Jason.

[PATCH v3 06/12] memory: mvebu-devbus: Remove address decoding window workaround

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 08:33:54AM -0400, Jason Cooper wrote:
> On Tue, Jun 18, 2013 at 02:17:31PM +0200, Thomas Petazzoni wrote:
> > Dear Jason Cooper,
> > 
> > On Tue, 18 Jun 2013 07:39:20 -0400, Jason Cooper wrote:
> > > On Tue, Jun 18, 2013 at 08:25:31AM -0300, Ezequiel Garcia wrote:
> > > > Now that mbus device tree binding has been introduced, remove the address
> > > > decoding window management from this driver.
> > > > A suitable 'ranges' entry should be added to the devbus-compatible node in
> > > > the device tree, as described by the mbus binding documentation.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > ---
> > > >  drivers/memory/mvebu-devbus.c | 64 ++-----------------------------------------
> > > >  1 file changed, 2 insertions(+), 62 deletions(-)
> > > 
> > > Unfortunately, the patch adding mvebu-devbus is going through gregkh's
> > > tree.  Either this patch needs to wait for v3.12, or ask Greg if he can
> > > take this one.
> > 
> > I believe that what we're looking for right now is a Acked-by from both
> > the Marvell maintainers, Arnd Bergmann and Jason Gunthorpe. Once we get
> > those Acked-by, we can figure out the merge strategy for this patch
> > set.
> 
> Right.  I was more looking to give Ezequiel a heads up

.. and you did :)

Thanks a lot,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 07/12] ARM: mvebu: Use the preprocessor on Armada 370/XP device tree files

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts              | 2 +-
 arch/arm/boot/dts/armada-370-mirabox.dts         | 2 +-
 arch/arm/boot/dts/armada-370-rd.dts              | 2 +-
 arch/arm/boot/dts/armada-370.dtsi                | 2 +-
 arch/arm/boot/dts/armada-xp-db.dts               | 2 +-
 arch/arm/boot/dts/armada-xp-gp.dts               | 2 +-
 arch/arm/boot/dts/armada-xp-mv78260.dtsi         | 2 +-
 arch/arm/boot/dts/armada-xp-mv78460.dtsi         | 2 +-
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 2 +-
 arch/arm/boot/dts/armada-xp.dtsi                 | 2 +-
 10 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index 2353b1f..231f8b2 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -14,7 +14,7 @@
  */
 
 /dts-v1/;
-/include/ "armada-370.dtsi"
+#include "armada-370.dtsi"
 
 / {
 	model = "Marvell Armada 370 Evaluation Board";
diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index 14e36e1..7005f27 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -9,7 +9,7 @@
  */
 
 /dts-v1/;
-/include/ "armada-370.dtsi"
+#include "armada-370.dtsi"
 
 / {
 	model = "Globalscale Mirabox";
diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index 130f839..5c3a1ec 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -12,7 +12,7 @@
  */
 
 /dts-v1/;
-/include/ "armada-370.dtsi"
+#include "armada-370.dtsi"
 
 / {
 	model = "Marvell Armada 370 Reference Design";
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index fa3dfc6..08ec6e3 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -15,7 +15,7 @@
  * common to all Armada SoCs.
  */
 
-/include/ "armada-370-xp.dtsi"
+#include "armada-370-xp.dtsi"
 /include/ "skeleton.dtsi"
 
 / {
diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index d6cc8bf..9a6ad0e 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -14,7 +14,7 @@
  */
 
 /dts-v1/;
-/include/ "armada-xp-mv78460.dtsi"
+#include "armada-xp-mv78460.dtsi"
 
 / {
 	model = "Marvell Armada XP Evaluation Board";
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 3ee63d1..e0c6f60 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -14,7 +14,7 @@
  */
 
 /dts-v1/;
-/include/ "armada-xp-mv78460.dtsi"
+#include "armada-xp-mv78460.dtsi"
 
 / {
 	model = "Marvell Armada XP Development Board DB-MV784MP-GP";
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index f4029f0..c80c60e 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -13,7 +13,7 @@
  * common to all Armada XP SoCs.
  */
 
-/include/ "armada-xp.dtsi"
+#include "armada-xp.dtsi"
 
 / {
 	model = "Marvell Armada XP MV78260 SoC";
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 488ca5e..3874548 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -13,7 +13,7 @@
  * common to all Armada XP SoCs.
  */
 
-/include/ "armada-xp.dtsi"
+#include "armada-xp.dtsi"
 
 / {
 	model = "Marvell Armada XP MV78460 SoC";
diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 46b7850..27a0ecf 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -11,7 +11,7 @@
  */
 
 /dts-v1/;
-/include/ "armada-xp-mv78260.dtsi"
+#include "armada-xp-mv78260.dtsi"
 
 / {
 	model = "PlatHome OpenBlocks AX3-4 board";
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 1ee8540..3e770db 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -16,7 +16,7 @@
  * common to all Armada SoCs.
  */
 
-/include/ "armada-370-xp.dtsi"
+#include "armada-370-xp.dtsi"
 
 / {
 	model = "Marvell Armada XP family SoC";
-- 
1.8.1.5

[PATCH v3 08/12] ARM: mvebu: Add MBus to Armada 370/XP device tree

[Ezequiel Garcia]

The Armada 370/XP SoC family has a completely configurable address
space handled by the MBus controller.

This patch introduces the device tree layout of MBus, making the
'soc' node as mbus-compatible.
Since every peripheral/controller is a child of this 'soc' node,
this makes all of them sit behind the mbus, thus describing the
hardware accurately.

A translation entry has been added for the internal-regs mapping.
This can't be done in the common armada-370-xp.dtsi because A370
and AXP have different addressing width.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts              | 2 ++
 arch/arm/boot/dts/armada-370-mirabox.dts         | 2 ++
 arch/arm/boot/dts/armada-370-rd.dts              | 2 ++
 arch/arm/boot/dts/armada-370-xp.dtsi             | 7 ++-----
 arch/arm/boot/dts/armada-370.dtsi                | 5 +++--
 arch/arm/boot/dts/armada-xp-db.dts               | 2 ++
 arch/arm/boot/dts/armada-xp-gp.dts               | 3 +--
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 3 +--
 arch/arm/boot/dts/armada-xp.dtsi                 | 3 +++
 9 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index 231f8b2..530d1cf 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -30,6 +30,8 @@
 	};
 
 	soc {
+		ranges = <0xffff0001 0 0xd0000000 0x100000>;
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <200000000>;
diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index 7005f27..ad60835 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -25,6 +25,8 @@
 	};
 
 	soc {
+		ranges = <0xffff0001 0 0xd0000000 0x100000>;
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <200000000>;
diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index 5c3a1ec..9c2ab7f 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -28,6 +28,8 @@
 	};
 
 	soc {
+		ranges = <0xffff0001 0 0xd0000000 0x100000>;
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <200000000>;
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 52a1f5e..633534dd 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -29,18 +29,15 @@
 	};
 
 	soc {
-		#address-cells = <1>;
+		#address-cells = <2>;
 		#size-cells = <1>;
-		compatible = "simple-bus";
 		interrupt-parent = <&mpic>;
-		ranges = <0          0 0xd0000000 0x0100000 /* internal registers */
-			  0xe0000000 0 0xe0000000 0x8100000 /* PCIe */>;
 
 		internal-regs {
 			compatible = "simple-bus";
 			#address-cells = <1>;
 			#size-cells = <1>;
-			ranges;
+			ranges = <0 0xffff0001 0 0x100000>;
 
 			mpic: interrupt-controller at 20000 {
 				compatible = "marvell,mpic";
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 08ec6e3..c274b7b 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -29,8 +29,9 @@
 	};
 
 	soc {
-		ranges = <0          0xd0000000 0x0100000 /* internal registers */
-			  0xe0000000 0xe0000000 0x8100000 /* PCIe */>;
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+
 		internal-regs {
 			system-controller at 18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index 9a6ad0e..0996215 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -30,6 +30,8 @@
 	};
 
 	soc {
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000>;
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <250000000>;
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index e0c6f60..f095a3e 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -39,8 +39,7 @@
 	};
 
 	soc {
-		ranges = <0          0 0xd0000000 0x100000
-			  0xf0000000 0 0xf0000000 0x1000000>;
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 27a0ecf..91827cb 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -27,8 +27,7 @@
 	};
 
 	soc {
-		ranges = <0          0 0xd0000000 0x100000
-			  0xf0000000 0 0xf0000000 0x8000000>;
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 3e770db..feec757 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -23,6 +23,9 @@
 	compatible = "marvell,armadaxp", "marvell,armada-370-xp";
 
 	soc {
+		compatible = "marvell,armadaxp-mbus", "simple-bus";
+		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
+
 		internal-regs {
 			L2: l2-cache {
 				compatible = "marvell,aurora-system-cache";
-- 
1.8.1.5

[PATCH v3 09/12] ARM: mvebu: Add BootROM to Armada 370/XP device tree

[Ezequiel Garcia]

In order to access the SoC BootROM, we need to declare a mapping
(through a ranges property). The mbus driver will use this property
to allocate a suitable address decoding window.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts              | 3 ++-
 arch/arm/boot/dts/armada-370-mirabox.dts         | 3 ++-
 arch/arm/boot/dts/armada-370-rd.dts              | 3 ++-
 arch/arm/boot/dts/armada-370-xp.dtsi             | 2 ++
 arch/arm/boot/dts/armada-370.dtsi                | 6 ++++++
 arch/arm/boot/dts/armada-xp-db.dts               | 3 ++-
 arch/arm/boot/dts/armada-xp-gp.dts               | 3 ++-
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 3 ++-
 arch/arm/boot/dts/armada-xp.dtsi                 | 6 ++++++
 9 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index 530d1cf..c313968 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -30,7 +30,8 @@
 	};
 
 	soc {
-		ranges = <0xffff0001 0 0xd0000000 0x100000>;
+		ranges = <0xffff0001 0 0xd0000000 0x100000
+			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index ad60835..abb1ccf 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -25,7 +25,8 @@
 	};
 
 	soc {
-		ranges = <0xffff0001 0 0xd0000000 0x100000>;
+		ranges = <0xffff0001 0 0xd0000000 0x100000
+			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index 9c2ab7f..9ae8bdc 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -28,7 +28,8 @@
 	};
 
 	soc {
-		ranges = <0xffff0001 0 0xd0000000 0x100000>;
+		ranges = <0xffff0001 0 0xd0000000 0x100000
+			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 633534dd..4ae5eb5 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -16,6 +16,8 @@
  * 370 and Armada XP SoC.
  */
 
+#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))
+
 /include/ "skeleton64.dtsi"
 
 / {
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index c274b7b..c7f9971 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -32,6 +32,12 @@
 		compatible = "marvell,armada370-mbus", "simple-bus";
 		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
 
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 MBUS_ID(0x01, 0xe0) 0 0xffffffff>;
+		};
+
 		internal-regs {
 			system-controller at 18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index 0996215..84e7ab2 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -30,7 +30,8 @@
 	};
 
 	soc {
-		ranges = <0xffff0001 0 0 0xd0000000 0x100000>;
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index f095a3e..1f4daa7 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -39,7 +39,8 @@
 	};
 
 	soc {
-		ranges = <0xffff0001 0 0 0xd0000000 0x100000>;
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 91827cb..ea47196 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -27,7 +27,8 @@
 	};
 
 	soc {
-		ranges = <0xffff0001 0 0 0xd0000000 0x100000>;
+		ranges = <0xffff0001 0 0 0xd0000000 0x100000
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
 
 		internal-regs {
 			serial at 12000 {
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index feec757..2516899 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -26,6 +26,12 @@
 		compatible = "marvell,armadaxp-mbus", "simple-bus";
 		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
 
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 MBUS_ID(0x01, 0x1d) 0 0xffffffff>;
+		};
+
 		internal-regs {
 			L2: l2-cache {
 				compatible = "marvell,aurora-system-cache";
-- 
1.8.1.5

[PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

[Ezequiel Garcia]

Now that mbus has been added to the device tree, it's possible to
move the DeviceBus out of internal registers, placing it directly
below the mbus. This is a more accurate representation of the hardware.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi             | 94 +++++++++++++-----------
 arch/arm/boot/dts/armada-xp-gp.dts               | 60 +++++++--------
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 60 +++++++--------
 3 files changed, 110 insertions(+), 104 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 4ae5eb5..0cd23ac 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -35,6 +35,56 @@
 		#size-cells = <1>;
 		interrupt-parent = <&mpic>;
 
+		devbus-bootcs {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10400 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs0 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10408 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x3e) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs1 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10410 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x3d) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs2 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10418 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x3b) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs3 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xffff0001 0x10420 0x8>;
+			ranges = <0 MBUS_ID(0x01, 0x37) 0 0xffffffff>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
 		internal-regs {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -181,50 +231,6 @@
 				status = "disabled";
 			};
 
-			devbus-bootcs at 10400 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10400 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs0 at 10408 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10408 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs1 at 10410 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10410 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs2 at 10418 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10418 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs3 at 10420 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10420 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
 		};
 	};
  };
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 1f4daa7..3e14e82 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -40,7 +40,36 @@
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
-			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
+
+		devbus-bootcs {
+			status = "okay";
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <8>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+
+			/* NOR 16 MiB */
+			nor at 0 {
+				compatible = "cfi-flash";
+				reg = <0 0x1000000>;
+				bank-width = <2>;
+			};
+		};
 
 		internal-regs {
 			serial at 12000 {
@@ -116,35 +145,6 @@
 				};
 			};
 
-			devbus-bootcs at 10400 {
-				status = "okay";
-				ranges = <0 0xf0000000 0x1000000>; /* @addr 0xf000000, size 0x1000000 */
-
-				/* Device Bus parameters are required */
-
-				/* Read parameters */
-				devbus,bus-width    = <8>;
-				devbus,turn-off-ps  = <60000>;
-				devbus,badr-skew-ps = <0>;
-				devbus,acc-first-ps = <124000>;
-				devbus,acc-next-ps  = <248000>;
-				devbus,rd-setup-ps  = <0>;
-				devbus,rd-hold-ps   = <0>;
-
-				/* Write parameters */
-				devbus,sync-enable = <0>;
-				devbus,wr-high-ps  = <60000>;
-				devbus,wr-low-ps   = <60000>;
-				devbus,ale-wr-ps   = <60000>;
-
-				/* NOR 16 MiB */
-				nor at 0 {
-					compatible = "cfi-flash";
-					reg = <0 0x1000000>;
-					bank-width = <2>;
-				};
-			};
-
 			pcie-controller {
 				status = "okay";
 
diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index ea47196..490c423 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -28,7 +28,36 @@
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
-			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;
+
+		devbus-bootcs {
+			status = "okay";
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <8>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+
+			/* NOR 128 MiB */
+			nor at 0 {
+				compatible = "cfi-flash";
+				reg = <0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
 
 		internal-regs {
 			serial at 12000 {
@@ -144,35 +173,6 @@
 				status = "okay";
 			};
 
-			devbus-bootcs at 10400 {
-				status = "okay";
-				ranges = <0 0xf0000000 0x8000000>; /* @addr 0xf000000, size 0x8000000 */
-
-				/* Device Bus parameters are required */
-
-				/* Read parameters */
-				devbus,bus-width    = <8>;
-				devbus,turn-off-ps  = <60000>;
-				devbus,badr-skew-ps = <0>;
-				devbus,acc-first-ps = <124000>;
-				devbus,acc-next-ps  = <248000>;
-				devbus,rd-setup-ps  = <0>;
-				devbus,rd-hold-ps   = <0>;
-
-				/* Write parameters */
-				devbus,sync-enable = <0>;
-				devbus,wr-high-ps  = <60000>;
-				devbus,wr-low-ps   = <60000>;
-				devbus,ale-wr-ps   = <60000>;
-
-				/* NOR 128 MiB */
-				nor at 0 {
-					compatible = "cfi-flash";
-					reg = <0 0x8000000>;
-					bank-width = <2>;
-				};
-			};
-
 			pcie-controller {
 				status = "okay";
 				/* Internal mini-PCIe connector */
-- 
1.8.1.5

[PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> 
> +               devbus-bootcs {
> +                       compatible = "marvell,mvebu-devbus";
> +                       reg = <0xffff0001 0x10400 0x8>;
> +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       clocks = <&coreclk 0>;
> +                       status = "disabled";
> +               };

This is a violation of the binding as far as I can tell, since you don't specify ranges
to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
the binding, I think you should clarify the binding and leave the implementation
as you have it here.

	Arnd

[PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > 
> > +               devbus-bootcs {
> > +                       compatible = "marvell,mvebu-devbus";
> > +                       reg = <0xffff0001 0x10400 0x8>;
> > +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       clocks = <&coreclk 0>;
> > +                       status = "disabled";
> > +               };
> 
> This is a violation of the binding as far as I can tell, since you don't specify ranges
> to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
> the binding, I think you should clarify the binding and leave the implementation
> as you have it here.
> 

Mmm... again I got lost here. Which 'ranges' you say I don't specify to
access the (formerly) 0xffff0001?

AFAIK, 'ranges' are only for children translation, which means I don't
need to specify a ranges for that in the devbus node, but in its parent,
right?

This ranges thing can be very tricky, so please correct me if I'm
mistaken.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 07:09:29PM -0300, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > 
> > > +               devbus-bootcs {
> > > +                       compatible = "marvell,mvebu-devbus";
> > > +                       reg = <0xffff0001 0x10400 0x8>;
> > > +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> > > +                       #address-cells = <1>;
> > > +                       #size-cells = <1>;
> > > +                       clocks = <&coreclk 0>;
> > > +                       status = "disabled";
> > > +               };
> > 
> > This is a violation of the binding as far as I can tell, since you don't specify ranges
> > to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
> > the binding, I think you should clarify the binding and leave the implementation
> > as you have it here.
> > 
> 
> Mmm... again I got lost here. Which 'ranges' you say I don't specify to
> access the (formerly) 0xffff0001?
> 

Oh, maybe you meant I'm not specifying an mbus-node ranges translation
in this same patch in this .dtsi file I'm modifying?

In that case, that's on purpose to avoid the nightmare involved in
mixing 'ranges' in per-board .dts files together with the ranges
declared in each included .dtsi.

By having only one mbus-node ranges property, per-board, it gets a bit
simpler.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

[Arnd Bergmann]

On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > 
> > > +               devbus-bootcs {
> > > +                       compatible = "marvell,mvebu-devbus";
> > > +                       reg = <0xffff0001 0x10400 0x8>;
> > > +                       ranges = <0 MBUS_ID(0x01, 0x2f) 0 0xffffffff>;
> > > +                       #address-cells = <1>;
> > > +                       #size-cells = <1>;
> > > +                       clocks = <&coreclk 0>;
> > > +                       status = "disabled";
> > > +               };
> > 
> > This is a violation of the binding as far as I can tell, since you don't specify ranges
> > to access the 0xffff0001 0x10400 address. However, as I explained in my comment for
> > the binding, I think you should clarify the binding and leave the implementation
> > as you have it here.
> > 
> 
> Mmm... again I got lost here. Which 'ranges' you say I don't specify to
> access the (formerly) 0xffff0001?
> 
> AFAIK, 'ranges' are only for children translation, which means I don't
> need to specify a ranges for that in the devbus node, but in its parent,
> right?
> 
> This ranges thing can be very tricky, so please correct me if I'm
> mistaken.

You already clarified that the binding was wrong. This was about the
part where you replied:

>> Do you really want to require the child to provide a "ranges" property?
>> I think this makes it more complicated to specify devices that belong
>> into the "internal-regs" category.
>> 
>
>No, this text is actually a left-over from the previous patchset, in
>current v3 patchset MBus children are not required to have any ranges.
>On the otherside, although you will need one except in the most trivial
>cases like for the BootROM.

With that change, everything above is ok.

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Ezequiel Garcia]

Now that mbus has been added to the device tree, it's possible to
move the PCIe nodes out of internal registers, placing it directly
below the mbus. This is a more accurate representation of the
hardware.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-db.dts      |  1 +
 arch/arm/boot/dts/armada-370-mirabox.dts | 33 +++++------
 arch/arm/boot/dts/armada-370-rd.dts      |  1 +
 arch/arm/boot/dts/armada-370.dtsi        | 97 ++++++++++++++++----------------
 4 files changed, 68 insertions(+), 64 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index c313968..6891343 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -31,6 +31,7 @@
 
 	soc {
 		ranges = <0xffff0001 0 0xd0000000 0x100000
+			  0xffff0002 0 0xe0000000 0x8100000
 			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
 		internal-regs {
diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index abb1ccf..830727a 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -26,8 +26,25 @@
 
 	soc {
 		ranges = <0xffff0001 0 0xd0000000 0x100000
+			  0xffff0002 0 0xe0000000 0x8100000
 			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
+		pcie-controller {
+			status = "okay";
+
+			/* Internal mini-PCIe connector */
+			pcie at 1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+
+			/* Connected on the PCB to a USB 3.0 XHCI controller */
+			pcie at 2,0 {
+				/* Port 1, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <200000000>;
@@ -122,22 +139,6 @@
 					reg = <0x25>;
 				};
 			};
-
-			pcie-controller {
-				status = "okay";
-
-				/* Internal mini-PCIe connector */
-				pcie at 1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-
-				/* Connected on the PCB to a USB 3.0 XHCI controller */
-				pcie at 2,0 {
-					/* Port 1, Lane 0 */
-					status = "okay";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index 9ae8bdc..132cf8e 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -29,6 +29,7 @@
 
 	soc {
 		ranges = <0xffff0001 0 0xd0000000 0x100000
+			  0xffff0002 0 0xe0000000 0x8100000
 			  MBUS_ID(0x01, 0xe0) 0 0xfff00000 0x100000>;
 
 		internal-regs {
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index c7f9971..1288a78 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -38,6 +38,55 @@
 			ranges = <0 MBUS_ID(0x01, 0xe0) 0 0xffffffff>;
 		};
 
+		pcie-controller {
+			compatible = "marvell,armada-370-pcie";
+			status = "disabled";
+			device_type = "pci";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			bus-range = <0x00 0xff>;
+
+			ranges =
+			       <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
+				0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
+				0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
+				0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;
+
+			pcie at 1,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
+				reg = <0x0800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 58>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 5>;
+				status = "disabled";
+			};
+
+			pcie at 2,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82002800 0 0x80000 0 0x2000>;
+				reg = <0x1000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 62>;
+				marvell,pcie-port = <1>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 9>;
+				status = "disabled";
+			};
+		};
+
 		internal-regs {
 			system-controller at 18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
@@ -176,54 +225,6 @@
 					0x18304 0x4>;
 				status = "okay";
 			};
-
-			pcie-controller {
-				compatible = "marvell,armada-370-pcie";
-				status = "disabled";
-				device_type = "pci";
-
-				#address-cells = <3>;
-				#size-cells = <2>;
-
-				bus-range = <0x00 0xff>;
-
-				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
-					0x82000000 0 0x80000 0x80000 0 0x00002000   /* Port 1.0 registers */
-					0x82000000 0 0xe0000000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
-					0x81000000 0 0          0xe8000000 0 0x00100000>; /* downstream I/O */
-
-				pcie at 1,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
-					reg = <0x0800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 58>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 5>;
-					status = "disabled";
-				};
-
-				pcie at 2,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82002800 0 0x80000 0 0x2000>;
-					reg = <0x1000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 62>;
-					marvell,pcie-port = <1>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 9>;
-					status = "disabled";
-				};
-			};
 		};
 	};
 };
-- 
1.8.1.5

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> +                       ranges =
> +                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> +                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> +                               0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
> +                               0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;

To clarify my earlier comment, I think it would be nicer to write this as

                       ranges =
                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
                               0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
                               0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
                               0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
                               0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;

The MBUS_ID numbers above are made up since I don't know them, but this way you can
describe how the entire 4GB MMIO address space of the PCI bus is mapped into the
MBUS address space.

> +                       pcie at 1,0 {
> +                               device_type = "pci";
> +                               assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
> +                               reg = <0x0800 0 0 0 0>;
> +                               #address-cells = <3>;
> +                               #size-cells = <2>;
> +                               #interrupt-cells = <1>;
> +                               ranges;

Then you do a ranges property for each port with the high-order
address word equal to the port number:

                     ranges = <0x82000000 1 0 0x82000000 0 0 1 0
                               0x81000000 1 0 0x81000000 0 0 0 0x10000>;


> +                               interrupt-map-mask = <0 0 0 0>;
> +                               interrupt-map = <0 0 0 0 &mpic 58>;
> +                               marvell,pcie-port = <0>;
> +                               marvell,pcie-lane = <0>;
> +                               clocks = <&gateclk 5>;
> +                               status = "disabled";
> +                       pcie at 2,0 {
> +                               device_type = "pci";
> +                               assigned-addresses = <0x82002800 0 0x80000 0 0x2000>;
> +                               reg = <0x1000 0 0 0 0>;
> +                               #address-cells = <3>;
> +                               #size-cells = <2>;
> +                               #interrupt-cells = <1>;
> +                               ranges;

                     ranges = <0x82000000 2 0 0x82000000 0 0 1 0
                               0x81000000 2 0 0x81000000 0 0 0 0x10000>;

> +                               interrupt-map-mask = <0 0 0 0>;
> +                               interrupt-map = <0 0 0 0 &mpic 62>;
> +                               marvell,pcie-port = <1>;
> +                               marvell,pcie-lane = <0>;
> +                               clocks = <&gateclk 9>;
> +                               status = "disabled";
> +                       };

Does this make sense?

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 18 Jun 2013 18:29:35 +0200, Arnd Bergmann wrote:

> To clarify my earlier comment, I think it would be nicer to write this as
> 
>                        ranges =
>                               <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
>                                0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
>                                0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
>                                0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
>                                0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
>                                0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;
> 
> The MBUS_ID numbers above are made up since I don't know them, but this way you can
> describe how the entire 4GB MMIO address space of the PCI bus is mapped into the
> MBUS address space.

This is *NOT* possible because we don't know in advance how much memory
space and I/O space each PCIe device will require.

Arnd, we've discussed this at length with you while getting the PCIe
driver merged, and we've explained this to you numerous times. Could
you please understand that *any* of your proposal that suggests writing
down static windows for PCIe devices will *not* work?

> Does this make sense?

Not at all. Please read once again the hundreds of e-mails we've
exchanged about the need for dynamic windows for PCIe devices, which
lead us to have the emulated PCI-to-PCI bridge stuff. I'm starting to
be fed up to re-explain this to you over-and-over again.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Thomas Petazzoni wrote:
> > To clarify my earlier comment, I think it would be nicer to write this as
> > 
> >                        ranges =
> >                               <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> >                                0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> >                                0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
> >                                0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
> >                                0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
> >                                0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;
> > 
> > The MBUS_ID numbers above are made up since I don't know them, but this way you can
> > describe how the entire 4GB MMIO address space of the PCI bus is mapped into the
> > MBUS address space.
> 
> This is NOT possible because we don't know in advance how much memory
> space and I/O space each PCIe device will require.
> 
> Arnd, we've discussed this at length with you while getting the PCIe
> driver merged, and we've explained this to you numerous times. Could
> you please understand that any of your proposal that suggests writing
> down static windows for PCIe devices will not work?

Where did I suggest static windows for PCIe devices?

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote:

> > >                        ranges =
> > >                               <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> > >                                0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> > >                                0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
> > >                                0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
> > >                                0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
> > >                                0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;
> > > 
> > > The MBUS_ID numbers above are made up since I don't know them, but this way you can
> > > describe how the entire 4GB MMIO address space of the PCI bus is mapped into the
> > > MBUS address space.
> > 
> > This is NOT possible because we don't know in advance how much memory
> > space and I/O space each PCIe device will require.
> > 
> > Arnd, we've discussed this at length with you while getting the PCIe
> > driver merged, and we've explained this to you numerous times. Could
> > you please understand that any of your proposal that suggests writing
> > down static windows for PCIe devices will not work?
> 
> Where did I suggest static windows for PCIe devices?

Where does your new proposal buys us anything useful compared to the
existing PCIe DT binding that has been discussed at length with you?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Thomas Petazzoni wrote:
> On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote:
> 
> > > >                        ranges =
> > > >                               <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> > > >                                0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> > > >                                0x82000000 1 0 MBUS_ID(0x12, 0x34) 0  1 0
> > > >                                0x82000000 2 0 MBUS_ID(0x13, 0x34) 0  1 0
> > > >                                0x81000000 1 0 MBUS_ID(0x12, 0x35) 0 0 0x10000;
> > > >                                0x81000000 2 0 MBUS_ID(0x13, 0x35) 0 0 0x10000>;
> > > > 
> > > > The MBUS_ID numbers above are made up since I don't know them, but this way you can
> > > > describe how the entire 4GB MMIO address space of the PCI bus is mapped into the
> > > > MBUS address space.
> > > 
> > > This is NOT possible because we don't know in advance how much memory
> > > space and I/O space each PCIe device will require.
> > > 
> > > Arnd, we've discussed this at length with you while getting the PCIe
> > > driver merged, and we've explained this to you numerous times. Could
> > > you please understand that any of your proposal that suggests writing
> > > down static windows for PCIe devices will not work?
> > 
> > Where did I suggest static windows for PCIe devices?
> 
> Where does your new proposal buys us anything useful compared to the
> existing PCIe DT binding that has been discussed at length with you?

I'm pretty sure I explained the idea above originally and was ignored.
Jason Gunthorpe might remember better, but I think he liked it when I
originally proposed doing it this way.

The main differences are:

* It unifies the binding for the PCIe case and all other registers. There is no
  need to treat PCIe special in the binding this way, which is more future-proof
  and more consistent.

* Since the host physical address for the PCIe memory space window is set up
  dynamically anyway, there is no reason to hardcode it in DT. We want it to
  be as large as possible, and this way the mbus driver can just pick the largest
  free area itself after setting up all the other mappings from the ranges
  property.

* The binding actually allows the PCIe translation to be discontiguous, so if
  we want to improve the code in the future, we can fill all the holes in the
  mbus space with PCIe windows without changing the binding.

* It separates hardware description (in the PCIe node) from policy (in the mbus
  node), just like we do for all the other mbus children.

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Jason Gunthorpe]

On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote:

> > > > Arnd, we've discussed this at length with you while getting the PCIe
> > > > driver merged, and we've explained this to you numerous times. Could
> > > > you please understand that any of your proposal that suggests writing
> > > > down static windows for PCIe devices will not work?
> > > 
> > > Where did I suggest static windows for PCIe devices?
> > 
> > Where does your new proposal buys us anything useful compared to the
> > existing PCIe DT binding that has been discussed at length with you?
> 
> I'm pretty sure I explained the idea above originally and was ignored.
> Jason Gunthorpe might remember better, but I think he liked it when I
> originally proposed doing it this way.

I remember it took a bit to understand your proposal, but I thought it
could work, but I admit I forget all the little details now :(

Ah, if I can just rephrase simply - the notion was to move the
determination of the aperture to use dynmic allocation and then
restructure the ranges around the mbus target, since they no longer
need to encode the aperture.

My concern: dynamically sizing the aperture is hard. There are three
apertures that need to be picked, and the PCI core code has no support
for dynamic apertures. Getting the aperture from the DT is a
functional compromise.

> * Since the host physical address for the PCIe memory space window
>   is set up dynamically anyway, there is no reason to hardcode it in
>   DT. We want it to be as large as possible, and this way the mbus
>   driver can just pick the largest free area itself after setting up
>   all the other mappings from the ranges property.

This seems to get really complicated if the mbus driver is ever
required to support dynamic mappings.. If PCI-E claims all memory and
then you modprobe something it could fail.

IMHO, I go back to my original thoughts. There is no real need for any
of this to be dynamic, we can use the values in the DT, presumably set
by the bootloader and things will work well.

The added complexity and failure modes for dynamic is simply not worth
it..

Jason

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Jason Gunthorpe wrote:
> On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote:
> 
> > > > > Arnd, we've discussed this at length with you while getting the PCIe
> > > > > driver merged, and we've explained this to you numerous times. Could
> > > > > you please understand that any of your proposal that suggests writing
> > > > > down static windows for PCIe devices will not work?
> > > > 
> > > > Where did I suggest static windows for PCIe devices?
> > > 
> > > Where does your new proposal buys us anything useful compared to the
> > > existing PCIe DT binding that has been discussed at length with you?
> > 
> > I'm pretty sure I explained the idea above originally and was ignored.
> > Jason Gunthorpe might remember better, but I think he liked it when I
> > originally proposed doing it this way.
> 
> I remember it took a bit to understand your proposal, but I thought it
> could work, but I admit I forget all the little details now :(
> 
> Ah, if I can just rephrase simply - the notion was to move the
> determination of the aperture to use dynmic allocation and then
> restructure the ranges around the mbus target, since they no longer
> need to encode the aperture.

Right.

> My concern: dynamically sizing the aperture is hard. There are three
> apertures that need to be picked, and the PCI core code has no support
> for dynamic apertures. Getting the aperture from the DT is a
> functional compromise.

After some discussion on IRC with Ezequiel, I think it's best to leave
the aperture listed in DT but say in the binding that the OS may
override it.

I would still want to see the actual MBUS IDs in the ranges property
of the pci-controller nodes. Right now, the pcie driver has to call
into the mbus driver passing a hardcoded string for setting up the
mapping, which is then used to look up the MBUS ID. This is rather
inconsistent and we should have all that information in DT. Ideally
we should also use it, to ensure it's correct but for 3.11 it would
be enough to get the DT representation right. We can replace the
string passing in 3.12.

> > * Since the host physical address for the PCIe memory space window
> >   is set up dynamically anyway, there is no reason to hardcode it in
> >   DT. We want it to be as large as possible, and this way the mbus
> >   driver can just pick the largest free area itself after setting up
> >   all the other mappings from the ranges property.
> 
> This seems to get really complicated if the mbus driver is ever
> required to support dynamic mappings.. If PCI-E claims all memory and
> then you modprobe something it could fail.

Yes, good point. However that is something we can figure out if it
comes to that. With my suggestion of setting up the mbus windows
in the of_bus xlate function, we would already create them at boot
time when of_platform_populate gets called, so that wouldn't be a
problem.

> IMHO, I go back to my original thoughts. There is no real need for any
> of this to be dynamic, we can use the values in the DT, presumably set
> by the bootloader and things will work well.
> 
> The added complexity and failure modes for dynamic is simply not worth
> it..

I don't think it's too hard to be prepared for fully dynamic operation.
As Grant said in his comment on v2, the real complexity comes from the
fact that we are mixing dynamic and static configuration here, and
the PCIe configuration is inherently dynamic.

The change I'm proposing would just mean the DT representation reflects
the dynamic nature of the PCIe windows.

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Ezequiel Garcia]

On Tue, Jun 18, 2013 at 11:20:07PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Jason Gunthorpe wrote:
> > On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote:
> > 
> > > > > > Arnd, we've discussed this at length with you while getting the PCIe
> > > > > > driver merged, and we've explained this to you numerous times. Could
> > > > > > you please understand that any of your proposal that suggests writing
> > > > > > down static windows for PCIe devices will not work?
> > > > > 
> > > > > Where did I suggest static windows for PCIe devices?
> > > > 
> > > > Where does your new proposal buys us anything useful compared to the
> > > > existing PCIe DT binding that has been discussed at length with you?
> > > 
> > > I'm pretty sure I explained the idea above originally and was ignored.
> > > Jason Gunthorpe might remember better, but I think he liked it when I
> > > originally proposed doing it this way.
> > 
> > I remember it took a bit to understand your proposal, but I thought it
> > could work, but I admit I forget all the little details now :(
> > 
> > Ah, if I can just rephrase simply - the notion was to move the
> > determination of the aperture to use dynmic allocation and then
> > restructure the ranges around the mbus target, since they no longer
> > need to encode the aperture.
> 
> Right.
> 
> > My concern: dynamically sizing the aperture is hard. There are three
> > apertures that need to be picked, and the PCI core code has no support
> > for dynamic apertures. Getting the aperture from the DT is a
> > functional compromise.
> 
> After some discussion on IRC with Ezequiel, I think it's best to leave
> the aperture listed in DT but say in the binding that the OS may
> override it.
> 

Yes, I'll send a v4 soon, and I'll try to address this correctly,
as you're suggesting.

[...]
> > IMHO, I go back to my original thoughts. There is no real need for any
> > of this to be dynamic, we can use the values in the DT, presumably set
> > by the bootloader and things will work well.
> > 
> > The added complexity and failure modes for dynamic is simply not worth
> > it..
> 
> I don't think it's too hard to be prepared for fully dynamic operation.
> As Grant said in his comment on v2, the real complexity comes from the
> fact that we are mixing dynamic and static configuration here, and
> the PCIe configuration is inherently dynamic.
> 
> The change I'm proposing would just mean the DT representation reflects
> the dynamic nature of the PCIe windows.
> 

Although I'd like the binding to take this into account, for there's no
point in restricting it -a priori- I can't see *any* advantage on doing
fully dynamic window configuration on devices that are fixed in the
first place. It sounds like bloating the whole thing without a strong
need.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> Although I'd like the binding to take this into account, for there's no
> point in restricting it -a priori- I can't see any advantage on doing
> fully dynamic window configuration on devices that are fixed in the
> first place. It sounds like bloating the whole thing without a strong
> need.

After the suggestions that Grant made about the of_bus, I think the
fully dynamic model would actually be simpler than what you have here.
You wouldn't actually have to dissect the "ranges" property at all,
just keep the mapping table in memory for all devices that are in use,
with a special case for the internal-regs.

But I think that's fine, we can alway simplify the code later as long
as the binding covers all cases.

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> +
> +                       ranges =
> +                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> +                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> +                               0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
> +                               0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;

As pointed out on IRC, this is not a good representation of the memory space,
since it requires a non-zero sys->mem_offset, and it conflicts with the straight
mapping I suggested.

I think it should be

                               0x82000000 0 0xe0000000 0xffff0002 0  0xe0000000 0x08000000

if we want to encode the aperture in the ranges property here, i.e. have
a 1:1 mapping between PCI memory space and MBUS space, and in mbus,
you need the corresponding

-                           0xffff0002 0 0xe0000000 0x8100000
+                           0xffff0002 0xe0000000 0xe0000000 0x8100000

so that mbus actually translates the right addresses. You could also
have the PCI memory space start at 0, which would mean

                               0x82000000 0 0 0xffff0002 0  0 0x08000000

and

                              0xffff0002 0 0xe0000000 0x8100000

Note that the driver doesn't actually handle the generic case correctly, you would
need to apply this patch
	
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..aa674f4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
 		}
 		if (restype == IORESOURCE_MEM) {
 			of_pci_range_to_resource(&range, np, &pcie->mem);
+			sys->mem_offset = range.cpu_addr - range.pci_addr;
 			pcie->mem.name = "MEM";
 		}
 	}

to deal with the generic case where the bus address is different from the
CPU address.

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Ezequiel Garcia]

Arnd,

Going through this a couple questions came out...

On Tue, Jun 18, 2013 at 11:35:50PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > +
> > +                       ranges =
> > +                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> > +                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> > +                               0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
> > +                               0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;
> 
> As pointed out on IRC, this is not a good representation of the memory space,
> since it requires a non-zero sys->mem_offset, and it conflicts with the straight
> mapping I suggested.
> 
> I think it should be
> 
>                                0x82000000 0 0xe0000000 0xffff0002 0  0xe0000000 0x08000000

So far, so good.

> 
> if we want to encode the aperture in the ranges property here, i.e. have
> a 1:1 mapping between PCI memory space and MBUS space, and in mbus,
> you need the corresponding
> 
> -                           0xffff0002 0 0xe0000000 0x8100000
> +                           0xffff0002 0xe0000000 0xe0000000 0x8100000

... I obviously got something wrong, but I thought you said that
this change allowed to *not* have any mbus-node ranges translation.

> 
> so that mbus actually translates the right addresses. You could also
> have the PCI memory space start at 0, which would mean
> 
>                                0x82000000 0 0 0xffff0002 0  0 0x08000000
> 
> and
> 
>                               0xffff0002 0 0xe0000000 0x8100000

Mmm.. and why is this option acceptable?

> 
> Note that the driver doesn't actually handle the generic case correctly, you would
> need to apply this patch
>

This patch does not apply. I tried to understand what you did, but I'm
confused by the fact I don't need to apply any patch to make it work
on my boxes, using your proposed ranges translations.

> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 13a633b..aa674f4 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
>  		}
>  		if (restype == IORESOURCE_MEM) {
>  			of_pci_range_to_resource(&range, np, &pcie->mem);
> +			sys->mem_offset = range.cpu_addr - range.pci_addr;
>  			pcie->mem.name = "MEM";
>  		}
>  	}
> 
> to deal with the generic case where the bus address is different from the
> CPU address.
> 

Thanks!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Wednesday 19 June 2013, Ezequiel Garcia wrote:
> Arnd,
> 
> Going through this a couple questions came out...
> 
> On Tue, Jun 18, 2013 at 11:35:50PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 June 2013, Ezequiel Garcia wrote:
> > > +
> > > +                       ranges =
> > > +                              <0x82000000 0 0x40000    0xffff0001 0x40000 0 0x00002000
> > > +                               0x82000000 0 0x80000    0xffff0001 0x80000 0 0x00002000
> > > +                               0x82000000 0 0xe0000000 0xffff0002 0          0 0x08000000
> > > +                               0x81000000 0 0          0xffff0002 0x8000000  0 0x00100000>;
> > 
> > As pointed out on IRC, this is not a good representation of the memory space,
> > since it requires a non-zero sys->mem_offset, and it conflicts with the straight
> > mapping I suggested.
> > 
> > I think it should be
> > 
> >                                0x82000000 0 0xe0000000 0xffff0002 0  0xe0000000 0x08000000
> 
> So far, so good.
> 
> > 
> > if we want to encode the aperture in the ranges property here, i.e. have
> > a 1:1 mapping between PCI memory space and MBUS space, and in mbus,
> > you need the corresponding
> > 
> > -                           0xffff0002 0 0xe0000000 0x8100000
> > +                           0xffff0002 0xe0000000 0xe0000000 0x8100000
> 
> ... I obviously got something wrong, but I thought you said that
> this change allowed to *not* have any mbus-node ranges translation.

I wasn't making a specific requirement here on whether you have that
in the mbus ranges or not, but if you put it in there, you have to adapt
the contents as above.



> > so that mbus actually translates the right addresses. You could also
> > have the PCI memory space start at 0, which would mean
> > 
> >                                0x82000000 0 0 0xffff0002 0  0 0x08000000
> > 
> > and
> > 
> >                               0xffff0002 0 0xe0000000 0x8100000
> 
> Mmm.. and why is this option acceptable?

As I explained on IRC, there is no requirement to pick a specific bus
aperture. The only two sensible choices are to make the bus address
the same as the CPU address, or to make the bus address start at 0,
which is what this does.

> > 
> > Note that the driver doesn't actually handle the generic case correctly, you would
> > need to apply this patch
> >
> 
> This patch does not apply. I tried to understand what you did, but I'm
> confused by the fact I don't need to apply any patch to make it work
> on my boxes, using your proposed ranges translations.
> 
> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> > index 13a633b..aa674f4 100644
> > --- a/drivers/pci/host/pci-mvebu.c
> > +++ b/drivers/pci/host/pci-mvebu.c
> > @@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
> >  		}
> >  		if (restype == IORESOURCE_MEM) {
> >  			of_pci_range_to_resource(&range, np, &pcie->mem);
> > +			sys->mem_offset = range.cpu_addr - range.pci_addr;
> >  			pcie->mem.name = "MEM";
> >  		}
> >  	}
> > 
> > to deal with the generic case where the bus address is different from the
> > CPU address.

This patch is needed only if you change the ranges to have the bus
aperture start at 0. However, if we pick a different way to specify
the aperture than putting it into both ranges properties, that point
is moot, since you wouldn't use of_pci_range_to_resource here
anyway.

	Arnd

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Jason Gunthorpe]

On Wed, Jun 19, 2013 at 02:11:58PM +0200, Arnd Bergmann wrote:

> > Mmm.. and why is this option acceptable?
> 
> As I explained on IRC, there is no requirement to pick a specific bus
> aperture. The only two sensible choices are to make the bus address
> the same as the CPU address, or to make the bus address start at 0,
> which is what this does.

PCI bus addresses must not alias other addresess in the system or
you'll get weirdness. For instance DMA initiated from the PCI bus at
address 0, intended to read from SDRAM at 0 must not be claimed by
another device on the PCI bus. IMHO, a 1:1 mapping between PCI and CPU
is strongly preferred. Any other configuration will need some
additional techniques to avoid aliasing.

Jason

[PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes

[Arnd Bergmann]

On Wednesday 19 June 2013, Jason Gunthorpe wrote:
> 
> Today 18:53:48
>    
> On Wed, Jun 19, 2013 at 02:11:58PM +0200, Arnd Bergmann wrote:
> 
> > > Mmm.. and why is this option acceptable?
> > 
> > As I explained on IRC, there is no requirement to pick a specific bus
> > aperture. The only two sensible choices are to make the bus address
> > the same as the CPU address, or to make the bus address start at 0,
> > which is what this does.
> 
> PCI bus addresses must not alias other addresess in the system or
> you'll get weirdness. For instance DMA initiated from the PCI bus at
> address 0, intended to read from SDRAM at 0 must not be claimed by
> another device on the PCI bus. IMHO, a 1:1 mapping between PCI and CPU
> is strongly preferred. Any other configuration will need some
> additional techniques to avoid aliasing.

Ah, good point. You are obviously right, it should definitely be a 1:1
mapping, anything else just creates a mess. I was working on a system
like that before, it wasn't pretty (you have to provide separate
dma_map_ops then).

	Arnd

[PATCH v3 12/12] ARM: mvebu: Relocate Armada XP PCIe device tree nodes

[Ezequiel Garcia]

Now that mbus has been added to the device tree, it's possible to
move the PCIe nodes out of internal registers, placing it directly
below the mbus. This is a more accurate representation of the
hardware.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-xp-db.dts               |  67 ++--
 arch/arm/boot/dts/armada-xp-gp.dts               |  43 +--
 arch/arm/boot/dts/armada-xp-mv78230.dtsi         | 209 ++++++-------
 arch/arm/boot/dts/armada-xp-mv78260.dtsi         | 245 +++++++--------
 arch/arm/boot/dts/armada-xp-mv78460.dtsi         | 377 ++++++++++++-----------
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |  19 +-
 6 files changed, 483 insertions(+), 477 deletions(-)

diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index 84e7ab2..95babaf 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -31,8 +31,42 @@
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
+			  0xffff0002 0 0 0xe0000000 0x8100000
 			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
 
+		pcie-controller {
+			status = "okay";
+
+			/*
+			 * All 6 slots are physically present as
+			 * standard PCIe slots on the board.
+			 */
+			pcie at 1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+			pcie at 2,0 {
+				/* Port 0, Lane 1 */
+				status = "okay";
+			};
+			pcie at 3,0 {
+				/* Port 0, Lane 2 */
+				status = "okay";
+			};
+			pcie at 4,0 {
+				/* Port 0, Lane 3 */
+				status = "okay";
+			};
+			pcie at 9,0 {
+				/* Port 2, Lane 0 */
+				status = "okay";
+			};
+			pcie at 10,0 {
+				/* Port 3, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <250000000>;
@@ -125,39 +159,6 @@
 					spi-max-frequency = <20000000>;
 				};
 			};
-
-			pcie-controller {
-				status = "okay";
-
-				/*
-				 * All 6 slots are physically present as
-				 * standard PCIe slots on the board.
-				 */
-				pcie at 1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-				pcie at 2,0 {
-					/* Port 0, Lane 1 */
-					status = "okay";
-				};
-				pcie at 3,0 {
-					/* Port 0, Lane 2 */
-					status = "okay";
-				};
-				pcie at 4,0 {
-					/* Port 0, Lane 3 */
-					status = "okay";
-				};
-				pcie at 9,0 {
-					/* Port 2, Lane 0 */
-					status = "okay";
-				};
-				pcie at 10,0 {
-					/* Port 3, Lane 0 */
-					status = "okay";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 3e14e82..266696f 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -40,6 +40,7 @@
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
+			  0xffff0002 0 0 0xe0000000 0x8100000
 			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
 			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
 
@@ -71,6 +72,27 @@
 			};
 		};
 
+		pcie-controller {
+			status = "okay";
+
+			/*
+			 * The 3 slots are physically present as
+			 * standard PCIe slots on the board.
+			 */
+			pcie at 1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+			pcie at 9,0 {
+				/* Port 2, Lane 0 */
+				status = "okay";
+			};
+			pcie at 10,0 {
+				/* Port 3, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <250000000>;
@@ -144,27 +166,6 @@
 					spi-max-frequency = <108000000>;
 				};
 			};
-
-			pcie-controller {
-				status = "okay";
-
-				/*
-				 * The 3 slots are physically present as
-				 * standard PCIe slots on the board.
-				 */
-				pcie at 1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-				pcie at 9,0 {
-					/* Port 2, Lane 0 */
-					status = "okay";
-				};
-				pcie at 10,0 {
-					/* Port 3, Lane 0 */
-					status = "okay";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78230.dtsi b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
index f8eaa38..5ac970d 100644
--- a/arch/arm/boot/dts/armada-xp-mv78230.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
@@ -44,6 +44,111 @@
 	};
 
 	soc {
+		/*
+		 * MV78230 has 2 PCIe units Gen2.0: One unit can be
+		 * configured as x4 or quad x1 lanes. One unit is
+		 * x4/x1.
+		 */
+		pcie-controller {
+			compatible = "marvell,armada-xp-pcie";
+			status = "disabled";
+			device_type = "pci";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			bus-range = <0x00 0xff>;
+
+			ranges =
+			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie at 1,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
+				reg = <0x0800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 58>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 5>;
+				status = "disabled";
+			};
+
+			pcie at 2,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
+				reg = <0x1000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 59>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <1>;
+				clocks = <&gateclk 6>;
+				status = "disabled";
+			};
+
+			pcie at 3,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x48000 0 0x2000>;
+				reg = <0x1800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 60>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <2>;
+				clocks = <&gateclk 7>;
+				status = "disabled";
+			};
+
+			pcie at 4,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x4c000 0 0x2000>;
+				reg = <0x2000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 61>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <3>;
+				clocks = <&gateclk 8>;
+				status = "disabled";
+			};
+
+			pcie at 9,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x42000 0 0x2000>;
+				reg = <0x4800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 99>;
+				marvell,pcie-port = <2>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 26>;
+				status = "disabled";
+			};
+		};
+
 		internal-regs {
 			pinctrl {
 				compatible = "marvell,mv78230-pinctrl";
@@ -77,110 +182,6 @@
 				#interrupts-cells = <2>;
 				interrupts = <87>, <88>, <89>;
 			};
-
-			/*
-			 * MV78230 has 2 PCIe units Gen2.0: One unit can be
-			 * configured as x4 or quad x1 lanes. One unit is
-			 * x4/x1.
-			 */
-			pcie-controller {
-				compatible = "marvell,armada-xp-pcie";
-				status = "disabled";
-				device_type = "pci";
-
-#address-cells = <3>;
-#size-cells = <2>;
-
-				bus-range = <0x00 0xff>;
-
-				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
-					0x82000000 0 0x42000 0x42000 0 0x00002000   /* Port 2.0 registers */
-					0x82000000 0 0x44000 0x44000 0 0x00002000   /* Port 0.1 registers */
-					0x82000000 0 0x48000 0x48000 0 0x00002000   /* Port 0.2 registers */
-					0x82000000 0 0x4c000 0x4c000 0 0x00002000   /* Port 0.3 registers */
-					0x82000000 0 0xe0000000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
-					0x81000000 0 0	  0xe8000000 0 0x00100000>; /* downstream I/O */
-
-				pcie at 1,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
-					reg = <0x0800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 58>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 5>;
-					status = "disabled";
-				};
-
-				pcie at 2,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
-					reg = <0x1000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 59>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <1>;
-					clocks = <&gateclk 6>;
-					status = "disabled";
-				};
-
-				pcie at 3,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x48000 0 0x2000>;
-					reg = <0x1800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 60>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <2>;
-					clocks = <&gateclk 7>;
-					status = "disabled";
-				};
-
-				pcie at 4,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x4c000 0 0x2000>;
-					reg = <0x2000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 61>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <3>;
-					clocks = <&gateclk 8>;
-					status = "disabled";
-				};
-
-				pcie at 9,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x42000 0 0x2000>;
-					reg = <0x4800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 99>;
-					marvell,pcie-port = <2>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 26>;
-					status = "disabled";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index c80c60e..ab06a45 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -45,6 +45,129 @@
 	};
 
 	soc {
+		/*
+		 * MV78260 has 3 PCIe units Gen2.0: Two units can be
+		 * configured as x4 or quad x1 lanes. One unit is
+		 * x4/x1.
+		 */
+		pcie-controller {
+			compatible = "marvell,armada-xp-pcie";
+			status = "disabled";
+			device_type = "pci";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			bus-range = <0x00 0xff>;
+
+			ranges =
+			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
+				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
+				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie at 1,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
+				reg = <0x0800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 58>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 5>;
+				status = "disabled";
+			};
+
+			pcie at 2,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
+				reg = <0x1000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 59>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <1>;
+				clocks = <&gateclk 6>;
+				status = "disabled";
+			};
+
+			pcie at 3,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x48000 0 0x2000>;
+				reg = <0x1800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 60>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <2>;
+				clocks = <&gateclk 7>;
+				status = "disabled";
+			};
+
+			pcie at 4,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x4c000 0 0x2000>;
+				reg = <0x2000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 61>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <3>;
+				clocks = <&gateclk 8>;
+				status = "disabled";
+			};
+
+			pcie at 9,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x42000 0 0x2000>;
+				reg = <0x4800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 99>;
+				marvell,pcie-port = <2>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 26>;
+				status = "disabled";
+			};
+
+			pcie at 10,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x82000 0 0x2000>;
+				reg = <0x5000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 103>;
+				marvell,pcie-port = <3>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 27>;
+				status = "disabled";
+			};
+		};
+
 		internal-regs {
 			pinctrl {
 				compatible = "marvell,mv78260-pinctrl";
@@ -97,128 +220,6 @@
 				clocks = <&gateclk 1>;
 				status = "disabled";
 			};
-
-			/*
-			 * MV78260 has 3 PCIe units Gen2.0: Two units can be
-			 * configured as x4 or quad x1 lanes. One unit is
-			 * x4/x1.
-			 */
-			pcie-controller {
-				compatible = "marvell,armada-xp-pcie";
-				status = "disabled";
-				device_type = "pci";
-
-				#address-cells = <3>;
-				#size-cells = <2>;
-
-				bus-range = <0x00 0xff>;
-
-				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
-					0x82000000 0 0x42000 0x42000 0 0x00002000   /* Port 2.0 registers */
-					0x82000000 0 0x44000 0x44000 0 0x00002000   /* Port 0.1 registers */
-					0x82000000 0 0x48000 0x48000 0 0x00002000   /* Port 0.2 registers */
-					0x82000000 0 0x4c000 0x4c000 0 0x00002000   /* Port 0.3 registers */
-					0x82000000 0 0x80000 0x80000 0 0x00002000   /* Port 1.0 registers */
-					0x82000000 0 0x82000 0x82000 0 0x00002000   /* Port 3.0 registers */
-					0x82000000 0 0xe0000000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
-					0x81000000 0 0	  0xe8000000 0 0x00100000>; /* downstream I/O */
-
-				pcie at 1,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
-					reg = <0x0800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 58>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 5>;
-					status = "disabled";
-				};
-
-				pcie at 2,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
-					reg = <0x1000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 59>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <1>;
-					clocks = <&gateclk 6>;
-					status = "disabled";
-				};
-
-				pcie at 3,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x48000 0 0x2000>;
-					reg = <0x1800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 60>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <2>;
-					clocks = <&gateclk 7>;
-					status = "disabled";
-				};
-
-				pcie at 4,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x4c000 0 0x2000>;
-					reg = <0x2000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 61>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <3>;
-					clocks = <&gateclk 8>;
-					status = "disabled";
-				};
-
-				pcie at 9,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x42000 0 0x2000>;
-					reg = <0x4800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 99>;
-					marvell,pcie-port = <2>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 26>;
-					status = "disabled";
-				};
-
-				pcie at 10,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x82000 0 0x2000>;
-					reg = <0x5000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 103>;
-					marvell,pcie-port = <3>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 27>;
-					status = "disabled";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 3874548..3d77133 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -60,6 +60,195 @@
 	};
 
 	soc {
+		/*
+		 * MV78460 has 4 PCIe units Gen2.0: Two units can be
+		 * configured as x4 or quad x1 lanes. Two units are
+		 * x4/x1.
+		 */
+		pcie-controller {
+			compatible = "marvell,armada-xp-pcie";
+			status = "disabled";
+			device_type = "pci";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			bus-range = <0x00 0xff>;
+
+			ranges =
+			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
+				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
+				0x82000000 0 0x84000 0xffff0001 0x84000 0 0x00002000   /* Port 1.1 registers */
+				0x82000000 0 0x88000 0xffff0001 0x88000 0 0x00002000   /* Port 1.2 registers */
+				0x82000000 0 0x8c000 0xffff0001 0x8c000 0 0x00002000   /* Port 1.3 registers */
+				0x82000000 0 0xe0000000 0xffff0002 0 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0 0xffff0002 0x8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie at 1,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
+				reg = <0x0800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 58>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 5>;
+				status = "disabled";
+			};
+
+			pcie at 2,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82001000 0 0x44000 0 0x2000>;
+				reg = <0x1000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 59>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <1>;
+				clocks = <&gateclk 6>;
+				status = "disabled";
+			};
+
+			pcie at 3,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82001800 0 0x48000 0 0x2000>;
+				reg = <0x1800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 60>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <2>;
+				clocks = <&gateclk 7>;
+				status = "disabled";
+			};
+
+			pcie at 4,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82002000 0 0x4c000 0 0x2000>;
+				reg = <0x2000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 61>;
+				marvell,pcie-port = <0>;
+				marvell,pcie-lane = <3>;
+				clocks = <&gateclk 8>;
+				status = "disabled";
+			};
+
+			pcie at 5,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82002800 0 0x80000 0 0x2000>;
+				reg = <0x2800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 62>;
+				marvell,pcie-port = <1>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 9>;
+				status = "disabled";
+			};
+
+			pcie at 6,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82003000 0 0x84000 0 0x2000>;
+				reg = <0x3000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 63>;
+				marvell,pcie-port = <1>;
+				marvell,pcie-lane = <1>;
+				clocks = <&gateclk 10>;
+				status = "disabled";
+			};
+
+			pcie at 7,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82003800 0 0x88000 0 0x2000>;
+				reg = <0x3800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 64>;
+				marvell,pcie-port = <1>;
+				marvell,pcie-lane = <2>;
+				clocks = <&gateclk 11>;
+				status = "disabled";
+			};
+
+			pcie at 8,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82004000 0 0x8c000 0 0x2000>;
+				reg = <0x4000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 65>;
+				marvell,pcie-port = <1>;
+				marvell,pcie-lane = <3>;
+				clocks = <&gateclk 12>;
+				status = "disabled";
+			};
+			pcie at 9,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82004800 0 0x42000 0 0x2000>;
+				reg = <0x4800 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 99>;
+				marvell,pcie-port = <2>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 26>;
+				status = "disabled";
+			};
+
+			pcie at 10,0 {
+				device_type = "pci";
+				assigned-addresses = <0x82005000 0 0x82000 0 0x2000>;
+				reg = <0x5000 0 0 0 0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				#interrupt-cells = <1>;
+				ranges;
+				interrupt-map-mask = <0 0 0 0>;
+				interrupt-map = <0 0 0 0 &mpic 103>;
+				marvell,pcie-port = <3>;
+				marvell,pcie-lane = <0>;
+				clocks = <&gateclk 27>;
+				status = "disabled";
+			};
+		};
+
 		internal-regs {
 			pinctrl {
 				compatible = "marvell,mv78460-pinctrl";
@@ -112,194 +301,6 @@
 				clocks = <&gateclk 1>;
 				status = "disabled";
 			};
-
-			/*
-			 * MV78460 has 4 PCIe units Gen2.0: Two units can be
-			 * configured as x4 or quad x1 lanes. Two units are
-			 * x4/x1.
-			 */
-			pcie-controller {
-				compatible = "marvell,armada-xp-pcie";
-				status = "disabled";
-				device_type = "pci";
-
-				#address-cells = <3>;
-				#size-cells = <2>;
-
-				bus-range = <0x00 0xff>;
-
-				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
-					0x82000000 0 0x42000 0x42000 0 0x00002000   /* Port 2.0 registers */
-					0x82000000 0 0x44000 0x44000 0 0x00002000   /* Port 0.1 registers */
-					0x82000000 0 0x48000 0x48000 0 0x00002000   /* Port 0.2 registers */
-					0x82000000 0 0x4c000 0x4c000 0 0x00002000   /* Port 0.3 registers */
-					0x82000000 0 0x80000 0x80000 0 0x00002000   /* Port 1.0 registers */
-					0x82000000 0 0x82000 0x82000 0 0x00002000   /* Port 3.0 registers */
-					0x82000000 0 0x84000 0x84000 0 0x00002000   /* Port 1.1 registers */
-					0x82000000 0 0x88000 0x88000 0 0x00002000   /* Port 1.2 registers */
-					0x82000000 0 0x8c000 0x8c000 0 0x00002000   /* Port 1.3 registers */
-					0x82000000 0 0xe0000000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
-					0x81000000 0 0	  0xe8000000 0 0x00100000>; /* downstream I/O */
-
-				pcie at 1,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
-					reg = <0x0800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 58>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 5>;
-					status = "disabled";
-				};
-
-				pcie at 2,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82001000 0 0x44000 0 0x2000>;
-					reg = <0x1000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 59>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <1>;
-					clocks = <&gateclk 6>;
-					status = "disabled";
-				};
-
-				pcie at 3,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82001800 0 0x48000 0 0x2000>;
-					reg = <0x1800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 60>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <2>;
-					clocks = <&gateclk 7>;
-					status = "disabled";
-				};
-
-				pcie at 4,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82002000 0 0x4c000 0 0x2000>;
-					reg = <0x2000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 61>;
-					marvell,pcie-port = <0>;
-					marvell,pcie-lane = <3>;
-					clocks = <&gateclk 8>;
-					status = "disabled";
-				};
-
-				pcie at 5,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82002800 0 0x80000 0 0x2000>;
-					reg = <0x2800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 62>;
-					marvell,pcie-port = <1>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 9>;
-					status = "disabled";
-				};
-
-				pcie at 6,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82003000 0 0x84000 0 0x2000>;
-					reg = <0x3000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 63>;
-					marvell,pcie-port = <1>;
-					marvell,pcie-lane = <1>;
-					clocks = <&gateclk 10>;
-					status = "disabled";
-				};
-
-				pcie at 7,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82003800 0 0x88000 0 0x2000>;
-					reg = <0x3800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 64>;
-					marvell,pcie-port = <1>;
-					marvell,pcie-lane = <2>;
-					clocks = <&gateclk 11>;
-					status = "disabled";
-				};
-
-				pcie at 8,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82004000 0 0x8c000 0 0x2000>;
-					reg = <0x4000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 65>;
-					marvell,pcie-port = <1>;
-					marvell,pcie-lane = <3>;
-					clocks = <&gateclk 12>;
-					status = "disabled";
-				};
-				pcie at 9,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82004800 0 0x42000 0 0x2000>;
-					reg = <0x4800 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 99>;
-					marvell,pcie-port = <2>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 26>;
-					status = "disabled";
-				};
-
-				pcie at 10,0 {
-					device_type = "pci";
-					assigned-addresses = <0x82005000 0 0x82000 0 0x2000>;
-					reg = <0x5000 0 0 0 0>;
-					#address-cells = <3>;
-					#size-cells = <2>;
-					#interrupt-cells = <1>;
-					ranges;
-					interrupt-map-mask = <0 0 0 0>;
-					interrupt-map = <0 0 0 0 &mpic 103>;
-					marvell,pcie-port = <3>;
-					marvell,pcie-lane = <0>;
-					clocks = <&gateclk 27>;
-					status = "disabled";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 490c423..591e2a1 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -28,6 +28,7 @@
 
 	soc {
 		ranges = <0xffff0001 0 0 0xd0000000 0x100000
+			  0xffff0002 0 0 0xe0000000 0x8100000
 			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
 			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;
 
@@ -59,6 +60,15 @@
 			};
 		};
 
+		pcie-controller {
+			status = "okay";
+			/* Internal mini-PCIe connector */
+			pcie at 1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial at 12000 {
 				clock-frequency = <250000000>;
@@ -172,15 +182,6 @@
 			usb at 51000 {
 				status = "okay";
 			};
-
-			pcie-controller {
-				status = "okay";
-				/* Internal mini-PCIe connector */
-				pcie at 1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-			};
 		};
 	};
 };
-- 
1.8.1.5

[PATCH v3 00/12] MBus device tree binding

[Sebastian Hesselbarth]

On 06/18/13 13:25, Ezequiel Garcia wrote:
> In the example below there's an extract of a device tree showing how
> the internal-regs and pcie nodes can be represented:
>
> 	#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))
>
> 	soc {
> 		compatible = "marvell,armadaxp-mbus";
> 		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
>
> 		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
> 			  0xffff0000 0 0 0xe0000000 0x8100000  /* pcie */
> 			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
> 			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;

Ezequiel,

you should update PCIE above (and possibly anywhere else) too.

> 		pcie-controller {
> 			compatible = "marvell,armada-xp-pcie";
> 			status = "okay";
> 			device_type = "pci";
>
> 			#address-cells = <3>;
> 			#size-cells = <2>;
>
> 			ranges =
> 			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
> 				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
> 				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
> 				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
> 				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
> 				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
> 				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
> 				0x82000000 0 0xe0000000 0xffff0000 0 0 0x08000000   /* non-prefetchable memory */
> 				0x81000000 0 0 0xffff0000 0x8000000 0 0x00100000>; /* downstream I/O */

Here for example.

> Changes from v2:
>   * Replaced the PCIe mapping with 0xffff0002, to avoid using a representation
>     that might correspond to a possible window id.

Out of curiosity, how will these fake mappings play with LPAE? If you
extend possible address space to >32b the lower bits may belong to valid
addresses.
Maybe you can (already do?) ignore that because of the 32b address
restriction for internal registers IIRC Thomas mentioned?

Sebastian

[PATCH v3 00/12] MBus device tree binding

[Ezequiel Garcia]

Hi Sebastian,

On Tue, Jun 18, 2013 at 01:33:37PM +0200, Sebastian Hesselbarth wrote:
> On 06/18/13 13:25, Ezequiel Garcia wrote:
> > In the example below there's an extract of a device tree showing how
> > the internal-regs and pcie nodes can be represented:
> >
> > 	#define MBUS_ID(target,attributes) (((target) << 24) | ((attributes) << 16))
> >
> > 	soc {
> > 		compatible = "marvell,armadaxp-mbus";
> > 		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
> >
> > 		ranges = <0xffff0001 0 0 0xd0000000 0x100000   /* internal-regs */
> > 			  0xffff0000 0 0 0xe0000000 0x8100000  /* pcie */
> > 			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
> > 			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>;
> 
> Ezequiel,
> 
> you should update PCIE above (and possibly anywhere else) too.
> 

Right. I've just checked and I only forgot to update the PCIe first-cell
(0xffff0000 -> 0xffff0002) in this cover-letter. The device tree files 
and the documentation look correct.

Sorry for the confusion!

> > 		pcie-controller {
> > 			compatible = "marvell,armada-xp-pcie";
> > 			status = "okay";
> > 			device_type = "pci";
> >
> > 			#address-cells = <3>;
> > 			#size-cells = <2>;
> >
> > 			ranges =
> > 			       <0x82000000 0 0x40000 0xffff0001 0x40000 0 0x00002000   /* Port 0.0 registers */
> > 				0x82000000 0 0x42000 0xffff0001 0x42000 0 0x00002000   /* Port 2.0 registers */
> > 				0x82000000 0 0x44000 0xffff0001 0x44000 0 0x00002000   /* Port 0.1 registers */
> > 				0x82000000 0 0x48000 0xffff0001 0x48000 0 0x00002000   /* Port 0.2 registers */
> > 				0x82000000 0 0x4c000 0xffff0001 0x4c000 0 0x00002000   /* Port 0.3 registers */
> > 				0x82000000 0 0x80000 0xffff0001 0x80000 0 0x00002000   /* Port 1.0 registers */
> > 				0x82000000 0 0x82000 0xffff0001 0x82000 0 0x00002000   /* Port 3.0 registers */
> > 				0x82000000 0 0xe0000000 0xffff0000 0 0 0x08000000   /* non-prefetchable memory */
> > 				0x81000000 0 0 0xffff0000 0x8000000 0 0x00100000>; /* downstream I/O */
> 
> Here for example.
> 
> > Changes from v2:
> >   * Replaced the PCIe mapping with 0xffff0002, to avoid using a representation
> >     that might correspond to a possible window id.
> 
> Out of curiosity, how will these fake mappings play with LPAE? If you
> extend possible address space to >32b the lower bits may belong to valid
> addresses.
> Maybe you can (already do?) ignore that because of the 32b address
> restriction for internal registers IIRC Thomas mentioned?
> 

That's a tricky question. The best explanation I can give you is that
these fake mappings belong to the MBus children address space, which is
different from the physical address space where LPAE comes into play.

This MBus children address space consists of two 32-bit cells:

IIAA00CC 00oooooo

The first one encodes the target ID and attribute in its upper 16 bits,
and custom values for the fake mappings in the lower 8 bits.
The second one encodes the offset into the MBus decoding window, and
this window can be as large as 4 GiB.

The address space I've just described is not the physical 64-bit address
space; it's the one where MBus children sit.

I hope I understood you're question, and I hope any of the above makes
sense!

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com