[PATCH 00/14] MBus device tree binding

[PATCH 00/14] MBus device tree binding

[Ezequiel Garcia]

From: Ezequiel Garcia <ezequiel-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

This patchset introduces the MBus DT binding.

The MBus DT binding is in charge of several different tasks:

  1. Allows the MBus driver to be probed through the Device Tree.

  2. Allocates statically declared decoding windows, such as those
     needed to access the BootROM or for the Device Bus children.

  3. Add 'ranges' translation entries as needed, to avoid duplicating
     the whole 'ranges' property in each per-board DTS file.

For the window static allocation, the current driver does that by walking
through its children and for each first-level child it creates decoding
windows from the translation entries.

For this to work, we need to encode the window's target ID and attribute ID
somewhere. The current design encodes it in an ad-hoc cell in the ranges
entry. See the binding documentation for a complete rationale.

This proposal has been written in its entirety following directions made by
Arnd Bergmann and Jason Gunthorpe when the MBus driver was introduced.
However, I'm open to any feedback and/or suggestions and ready to re-write
the whole thing, if it's a too-wacky solution.

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"

Ezequiel Garcia (14):
  bus: mvebu-mbus: Use pr_fmt
  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
  bus: mvebu-mbus: Update the mbus-compatible node's ranges property
  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: 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: Remove device tree unused properties on A370
  ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
  ARM: mvebu: Relocate Armada XP PCIe device tree nodes

 .../devicetree/bindings/bus/mvebu-mbus.txt         | 151 +++++++++
 arch/arm/boot/dts/armada-370-mirabox.dts           |  32 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               |  96 +++---
 arch/arm/boot/dts/armada-370.dtsi                  | 113 +++---
 arch/arm/boot/dts/armada-xp-db.dts                 |  66 ++--
 arch/arm/boot/dts/armada-xp-gp.dts                 | 101 +++---
 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   |  77 ++---
 arch/arm/boot/dts/armada-xp.dtsi                   |  10 +
 arch/arm/mach-mvebu/armada-370-xp.c                |  34 +-
 arch/arm/mach-mvebu/platsmp.c                      |   1 -
 drivers/bus/mvebu-mbus.c                           | 332 ++++++++++++++++--
 drivers/memory/mvebu-devbus.c                      |  64 +---
 include/linux/mbus.h                               |   1 +
 16 files changed, 1129 insertions(+), 780 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/mvebu-mbus.txt

-- 
1.8.1.5

[PATCH 01/14] bus: mvebu-mbus: Use pr_fmt

[Ezequiel Garcia]

In order to clean message printing, we replace pr_info with pr_fmt.
This is purely cosmetic change, with the sole purpose of making
the code a bit more readable.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/bus/mvebu-mbus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 8740f46..33c6947 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -49,6 +49,8 @@
  *   configuration (file 'devices').
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -762,7 +764,7 @@ int mvebu_mbus_add_window_remap_flags(const char *devname, phys_addr_t base,
 			break;
 
 	if (!s->soc->map[i].name) {
-		pr_err("mvebu-mbus: unknown device '%s'\n", devname);
+		pr_err("unknown device '%s'\n", devname);
 		return -ENODEV;
 	}
 
@@ -775,7 +777,7 @@ int mvebu_mbus_add_window_remap_flags(const char *devname, phys_addr_t base,
 		attr |= 0x28;
 
 	if (!mvebu_mbus_window_conflicts(s, base, size, target, attr)) {
-		pr_err("mvebu-mbus: cannot add window '%s', conflicts with another window\n",
+		pr_err("cannot add window '%s', conflicts with another window\n",
 		       devname);
 		return -EINVAL;
 	}
@@ -842,7 +844,7 @@ int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base,
 			break;
 
 	if (!of_id->compatible) {
-		pr_err("mvebu-mbus: could not find a matching SoC family\n");
+		pr_err("could not find a matching SoC family\n");
 		return -ENODEV;
 	}
 
-- 
1.8.1.5

Re: [PATCH 01/14] bus: mvebu-mbus: Use pr_fmt

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Fri,  7 Jun 2013 13:47:38 -0300, Ezequiel Garcia wrote:
> In order to clean message printing, we replace pr_info with pr_fmt.
> This is purely cosmetic change, with the sole purpose of making
> the code a bit more readable.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

I believe this one could be applied regardless of what happens to the
rest of the patch series.

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

Re: [PATCH 01/14] bus: mvebu-mbus: Use pr_fmt

[Jason Cooper]

On Fri, Jun 07, 2013 at 01:47:38PM -0300, Ezequiel Garcia wrote:
> In order to clean message printing, we replace pr_info with pr_fmt.
> This is purely cosmetic change, with the sole purpose of making
> the code a bit more readable.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/bus/mvebu-mbus.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

As Thomas mentioned, this patch has been applied to mvebu/cleanup with
his Ack.

thx,

Jason.

[PATCH 02/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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 03/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Arnd Bergmann]

On Friday 07 June 2013, Ezequiel Garcia wrote:
> +       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;
> +       }

Just an idea to make this more regular: Since the internal-regs can no longer
be regarded as a fixed location, we might want to use the same "ranges" property
mechanism for resolving the internal regs as we use for everything else.

This would imply that the device node this driver binds to would actually
end up being a child of the bus itself, which then goes on to modify the
ranges property of its parent node. Does that make sense?

	Arnd

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Jason Gunthorpe]

On Fri, Jun 07, 2013 at 09:10:35PM +0200, Arnd Bergmann wrote:
> On Friday 07 June 2013, Ezequiel Garcia wrote:
> > +       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;
> > +       }
> 
> Just an idea to make this more regular: Since the internal-regs can no longer
> be regarded as a fixed location, we might want to use the same "ranges" property
> mechanism for resolving the internal regs as we use for everything else.
> 
> This would imply that the device node this driver binds to would actually
> end up being a child of the bus itself, which then goes on to modify the
> ranges property of its parent node. Does that make sense?

We have a minimum requirement that the bootloader setup internal regs,
so the minimum required DT bindings is going to be this:

mbus {
  compatible = ...
  ranges = <INTERNAL_REGS_MAP_ID 0xf1000000 0x100000>;
  reg = <0xf1000xxx ...>; // MBUS regs block
  #address/size-cells...

  internal-regs  {
      ranges = <0 INTERNAL_REGS_MAP_ID 0x100000>;
  }
}

ie the ranges should never be empty.

Discovery of the address of the mbus control registers is via the reg
property on its own node (which is untranslated), and all other
internal regs blocks will automatically translate as they are supposed
to (Thomas's work to make internal regs 0xd/0xf1 relies on this, AFAIK)

INTERNAL_REGS_MAP_ID is an invalid target ID value, defined to mean
the internal registers target. 0xFFFFFFFF is a better choice for this
than 0, because 0xFFFFFFFF is never going to be a valid target id, it
is too large.

Jason

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Arnd Bergmann]

On Friday 07 June 2013, Jason Gunthorpe wrote:
> We have a minimum requirement that the bootloader setup internal regs,
> so the minimum required DT bindings is going to be this:
> 
> mbus {
>   compatible = ...
>   ranges = <INTERNAL_REGS_MAP_ID 0xf1000000 0x100000>;
>   reg = <0xf1000xxx ...>; // MBUS regs block
>   #address/size-cells...
> 
>   internal-regs  {
>       ranges = <0 INTERNAL_REGS_MAP_ID 0x100000>;
>   }
> }
> 
> ie the ranges should never be empty.

Right, that's what I thought.

> Discovery of the address of the mbus control registers is via the reg
> property on its own node (which is untranslated), and all other
> internal regs blocks will automatically translate as they are supposed
> to (Thomas's work to make internal regs 0xd/0xf1 relies on this, AFAIK)
> 
> INTERNAL_REGS_MAP_ID is an invalid target ID value, defined to mean
> the internal registers target. 0xFFFFFFFF is a better choice for this
> than 0, because 0xFFFFFFFF is never going to be a valid target id, it
> is too large.

Can you explain to me why it is an invalid target ID value? Is it
treated very differently by the mbus register setup than all the
others? I guess we can define it as something else to make a valid
target ID, by using one or more of the remaining bits in the first
address cell.

	Arnd

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Jason Gunthorpe]

On Fri, Jun 07, 2013 at 09:53:03PM +0200, Arnd Bergmann wrote:
 
> Can you explain to me why it is an invalid target ID value? Is it
> treated very differently by the mbus register setup than all the
> others? I guess we can define it as something else to make a valid
> target ID, by using one or more of the remaining bits in the first
> address cell.

There is a special dedicated window register for the internal regs
base. Marvell did not define it a target ID or window attributes, and
only that window register can be used to configure it. The mbus driver
should never read or write this register.

So we have a 2 cell address encoding of the form:

 IIAA0000 00oooooo

Where:
 -- I = Marvell defined target ID for programmable windows
 -- A = Marvell defined target attributes for programmable windows
 -- o = offset within the window

Now, we need 2 cell format that tells the driver that this is the
internal regs block. Assuming that marvell defines all I/A values we
need another value (I called it an invalid valid) to indicate internal
regs, several choices:

  FFFFFFFF 00oooooo
  0000FFFF 00oooooo
  00000001 00oooooo

Doesn't really matter which is picked, as far as I can tell, though
maybe the 01 option is best, leading to:

  IIAAssss 00oooooo

Where:
 -- s = target space
   == 0 means I/A are:
    -- I = Marvell defined target ID for programmable windows
    -- A = Marvell defined target attributes for programmable windows
   == 1 means internal regs block
 -- o = offset within the target

Which is pretty tidy..

Jason

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Arnd Bergmann]

On Friday 07 June 2013, Jason Gunthorpe wrote:
> 
> On Fri, Jun 07, 2013 at 09:53:03PM +0200, Arnd Bergmann wrote:
>  
> > Can you explain to me why it is an invalid target ID value? Is it
> > treated very differently by the mbus register setup than all the
> > others? I guess we can define it as something else to make a valid
> > target ID, by using one or more of the remaining bits in the first
> > address cell.
> 
> There is a special dedicated window register for the internal regs
> base. Marvell did not define it a target ID or window attributes, and
> only that window register can be used to configure it.

Ok.

> The mbus driver should never read or write this register.

That is not a hard requirement, right? I guess based on the
recent discussion about the 0xd0 or 0xf1 window, there may
actually be good reasons to reassign it, although I agree
that we shouldn't bother with implementing that for now, as
it's not a simple or urgent problem.

> So we have a 2 cell address encoding of the form:
> 
>  IIAA0000 00oooooo
> 
> Where:
>  -- I = Marvell defined target ID for programmable windows
>  -- A = Marvell defined target attributes for programmable windows
>  -- o = offset within the window
> 
> Now, we need 2 cell format that tells the driver that this is the
> internal regs block. Assuming that marvell defines all I/A values we
> need another value (I called it an invalid valid) to indicate internal
> regs, several choices:
> 
>   FFFFFFFF 00oooooo
>   0000FFFF 00oooooo
>   00000001 00oooooo
> 
> Doesn't really matter which is picked, as far as I can tell, though
> maybe the 01 option is best, leading to:
> 
>   IIAAssss 00oooooo
> 
> Where:
>  -- s = target space
>    == 0 means I/A are:
>     -- I = Marvell defined target ID for programmable windows
>     -- A = Marvell defined target attributes for programmable windows
>    == 1 means internal regs block
>  -- o = offset within the target
> 
> Which is pretty tidy..

Ok, thanks for the explanation. I think this is a good representation.
The other idea I raised in my previous mail would end up with something
like

 SIIAA000 00oooooo

with 'S' having the inverted meaning of your 's', so '0' actually
refers to the internal regs as in Ezequiel's current code.

I'm fine with either representation though.


	Arnd

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Jason Gunthorpe]

On Fri, Jun 07, 2013 at 11:15:50PM +0200, Arnd Bergmann wrote:

> > The mbus driver should never read or write this register.
> 
> That is not a hard requirement, right? I guess based on the
> recent discussion about the 0xd0 or 0xf1 window, there may
> actually be good reasons to reassign it, although I agree
> that we shouldn't bother with implementing that for now, as
> it's not a simple or urgent problem.

Yes, exactly.

To be meaningfull any re-assignment probably has to go along with a
SDRAM remap as well, which is tricky to impossible to do when you are
already running from SDRAM.

> > Doesn't really matter which is picked, as far as I can tell, though
> > maybe the 01 option is best, leading to:
> > 
> >   IIAAssss 00oooooo
> > 
> > Where:
> >    == 0 means I/A are:
> >    == 1 means internal regs block
> > 
> > Which is pretty tidy..
> 
> Ok, thanks for the explanation. I think this is a good representation.
> The other idea I raised in my previous mail would end up with something
> like
> 
>  SIIAA000 00oooooo
> 
> with 'S' having the inverted meaning of your 's', so '0' actually
> refers to the internal regs as in Ezequiel's current code.
> 
> I'm fine with either representation though.

Right, it doesn't really matter, Ezequiel should pick whatever he
likes, as long as the encoding is fully described in the binding
document (Ezequiel, feel free to crib my text), and the 'special' IDs
don't overlap with potentially valid target ids (ie don't use 0 ==
internal regs).

Jason

Re: [PATCH 03/14] bus: mvebu-mbus: Introduce device tree binding

[Ezequiel Garcia]

On Fri, Jun 07, 2013 at 01:44:30PM -0600, Jason Gunthorpe wrote:
> On Fri, Jun 07, 2013 at 09:10:35PM +0200, Arnd Bergmann wrote:
> > On Friday 07 June 2013, Ezequiel Garcia wrote:
> > > +       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;
> > > +       }
> > 
> > Just an idea to make this more regular: Since the internal-regs can no longer
> > be regarded as a fixed location, we might want to use the same "ranges" property
> > mechanism for resolving the internal regs as we use for everything else.
> > 
> > This would imply that the device node this driver binds to would actually
> > end up being a child of the bus itself, which then goes on to modify the
> > ranges property of its parent node. Does that make sense?
> 
> We have a minimum requirement that the bootloader setup internal regs,
> so the minimum required DT bindings is going to be this:
> 
> mbus {
>   compatible = ...
>   ranges = <INTERNAL_REGS_MAP_ID 0xf1000000 0x100000>;
>   reg = <0xf1000xxx ...>; // MBUS regs block
>   #address/size-cells...
> 
>   internal-regs  {
>       ranges = <0 INTERNAL_REGS_MAP_ID 0x100000>;
>   }
> }
> 

... and the above is the way its done in my proposal,
with INTERNAL_REGS_MAP_ID = 0x0.

> ie the ranges should never be empty.
> 
> Discovery of the address of the mbus control registers is via the reg
> property on its own node (which is untranslated), and all other
> internal regs blocks will automatically translate as they are supposed
> to (Thomas's work to make internal regs 0xd/0xf1 relies on this, AFAIK)
> 
> INTERNAL_REGS_MAP_ID is an invalid target ID value, defined to mean
> the internal registers target. 0xFFFFFFFF is a better choice for this
> than 0, because 0xFFFFFFFF is never going to be a valid target id, it
> is too large.
> 

I agree, using 0x0 was not a good choice. I'll change that.

Thanks both for the feedback!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 04/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/bus/mvebu-mbus.txt         | 152 +++++++++++++++++++++
 drivers/bus/mvebu-mbus.c                           | 110 ++++++++++++++-
 2 files changed, 261 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..2b0303e
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
@@ -0,0 +1,152 @@
+
+* 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.
+
+Example:
+
+	soc {
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+	};
+
+** How does it work?
+
+The MBus driver controls the allocation and release of the addresses
+decoding windows needed to access devices.
+
+Each window, is identified by its target ID and attribute ID. In order
+to represent this, each of mbus-node first-level child has to declare
+a suitable 'ranges' translation entry for the MBus to allocate a window
+for it. This entry will encode the target and attribute in a 'windowid'
+ad-hoc cell, like this:
+
+	soc {
+		bootrom {
+			ranges = <0 0x01e00000 0xfff00000 0x100000>;
+		};
+	};
+
+In this case, the MBus driver will allocate a window with target ID = 0x01,
+attribute ID = 0xe0, base address 0xfff00000, and size 0x100000.
+The windowid cell encodes the target and attribute in the upper bytes.
+
+Note that, the above 'ranges' property is creating a fictitious 2-cell address
+space with the windowid and the base address. We need to get rid of this
+2-cell address space, translating it into a real address space consisting of
+just the base address.
+
+The obvious way is to add a translation at the mbus-node level, like this:
+
+	soc {
+		ranges = <0x01e00000 0xfff00000 0xfff00000 0x100000>;
+		bootrom {
+			ranges = <0 0x01e00000 0xfff00000 0x100000>;
+		};
+	};
+
+This would lead to a lot of 'ranges' properties duplication because
+when you need to declare a new translation for a given device, you have
+to do it at the per-board DTS file. But since 'ranges' entries are not
+inherited from included dtsi files, you would have to go through each of the
+included dtsi files, making sure you duplicate each of the previous entries.
+
+So to avoid this duplication, the MBus driver is capable of adding the
+required translation entry to the device tree programatically. In other
+words, when writing the device tree you don't need to extend the mbus-node
+translation entry list; the driver will do it dynamically.
+
+So for instance, if you want to add support for a NOR device, that sits
+behind the Device Bus controller, you would have this device tree:
+(Note that the windowid cell is encoding the target ID = 0x01 and attribute
+ID = 0x2f, and the selected base address for the window is 0xe8000000).
+
+	soc {
+		devbus-bootcs {
+			status = "okay";
+			ranges = <0 0x012f0000 0xe8000000 0x8000000>;
+
+			/* Device Bus parameters are required but not shown */
+			/* ... */
+
+			/* NOR 128 MiB */
+			nor@0 {
+				compatible = "cfi-flash";
+				reg = <0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
+	};
+
+** 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.
+
+** About the special target ID and attribute ID
+
+As stated above, for each 'ranges' translation entry in mbus-node first-level
+children, the MBus driver will allocate a decoding window.
+However, in some cases it's desirable to declare a translation entry but have
+the MBus driver skip it.
+
+This is currently the case for the internal-regs and the pcie-controller
+nodes.
+
+In the former, a dummy translation entry is needed to map the regular device
+nodes into the {windowid / base address} address space, using a zeroed windowid
+(target = 0x0, attribute = 0x0). The MBus driver will skip entries with such
+windowid.
+
+	soc {
+		/* ... */
+		internal-regs {
+			compatible = "simple-bus";
+			ranges = <0 0 0 0x100000>;  /* Dummy translation */
+			serial@12000 {
+				reg = <0x12000 0x100>;
+				/* ... */
+			};
+		};
+	};
+
+In the pcie-controller case, we have two kinds of entries we need to skip.
+The first one is identical to the internal-regs case; in fact it's declaring a
+translation into the internal-regs address space.
+
+The second one is identified by a target ID = 0xff and attribute ID = 0xff.
+We use this to create the mapping into the large region where the PCIe
+controller will request decoding window allocations from the MBus driver.
+
+	soc {
+		pcie-controller {
+			ranges =
+			       <0x82000000 0 0x40000    0          0x40000    0 0x00002000
+				0x82000000 0 0x80000    0          0x80000    0 0x00002000
+				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000
+				0x81000000 0 0          0xffff0000 0xe8000000 0 0x00100000>;
+
+			pcie@1,0 {
+				/* ... */
+			};
+		};
+	};
+
+We don't want the MBus driver to statically allocate windows for this region,
+because the PCIe controller itself will make such requests depending on what
+kind of PCIe devices are detected.
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 23f6ae6..ac4115a 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -885,6 +885,110 @@ 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  0 to 15: unused
+ */
+#define TARGET(id) (((id) & 0xFF000000) >> 24)
+#define ATTR(id)   (((id) & 0x00FF0000) >> 16)
+
+static int __init mbus_dt_setup_win(struct mvebu_mbus_state *mbus,
+				    u32 windowid, u32 base, u32 size,
+				    u8 target, u8 attr)
+{
+	const struct mvebu_mbus_mapping *map = mbus->soc->map;
+	const char *name;
+	int i;
+
+	/* Special case for the identity mapping */
+	if (target == 0xff && attr == 0xff)
+		return 0;
+
+	/* Special case for the internal registers */
+	if (target == 0x00 && attr == 0x00)
+		return 0;
+
+	/* 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_dt_setup(struct mvebu_mbus_state *mbus,
+				struct device_node *np)
+{
+	struct device_node *child;
+	int ranges_len, tuple_len;
+	int addr_cells, c_addr_cells, c_size_cells;
+	int cell_count;
+	const __be32 *r, *ranges_start, *ranges_end, *prop;
+
+	for_each_available_child_of_node(np, child) {
+
+		addr_cells = of_n_addr_cells(child);
+
+		prop = of_get_property(child, "#address-cells", NULL);
+		c_addr_cells = be32_to_cpup(prop);
+
+		prop = of_get_property(child, "#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(child, "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", child->name);
+			continue;
+		}
+
+		for (r = ranges_start; r < ranges_end; r += cell_count) {
+			u32 windowid, base, size;
+			u8 target, attr;
+			int ret;
+
+			windowid = of_read_number(r + c_addr_cells, 1);
+			base = of_read_number(r + c_addr_cells + 1, 1);
+			size = of_read_number(r + addr_cells + c_addr_cells,
+					      c_size_cells);
+			target = TARGET(windowid);
+			attr = ATTR(windowid);
+
+			ret = mbus_dt_setup_win(mbus, windowid, 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 +1020,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

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Friday 07 June 2013, Ezequiel Garcia wrote:
> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

I think this is very good in general, but in some cases should be reworded
for clarity, and in a few others we should remove the special cases from
the binding and try to treat them more like the common case.

>  .../devicetree/bindings/bus/mvebu-mbus.txt         | 152 +++++++++++++++++++++
>  drivers/bus/mvebu-mbus.c                           | 110 ++++++++++++++-
>  2 files changed, 261 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..2b0303e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/mvebu-mbus.txt
> @@ -0,0 +1,152 @@
> +
> +* Marvell MBus controller
> +
> +Required properties:
> +
> +- compatible:	Should be set to one of the following:
> +		marvell,armada370-mbus
> +		marvell,armadaxp-mbus

I thought they are compatible with all older ones at the register level,
as long as we describe all the differences in the ranges property
and other properties.

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

I think you should also list #address-cells=<2> and #size-cells=<1>
as required here, as long as an optional "ranges" property.

I think it makes sense to describe the "ranges" format here,
even if we expect the property to be empty at boot time and
only filled at run-time.

> +Example:
> +
> +	soc {
> +		compatible = "marvell,armada370-mbus", "simple-bus";
> +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> +	};
> +
> +** How does it work?
> +
> +The MBus driver controls the allocation and release of the addresses
> +decoding windows needed to access devices.
> +
> +Each window, is identified by its target ID and attribute ID. In order
> +to represent this, each of mbus-node first-level child has to declare
> +a suitable 'ranges' translation entry for the MBus to allocate a window
> +for it. This entry will encode the target and attribute in a 'windowid'
> +ad-hoc cell, like this:
> +
> +	soc {
> +		bootrom {
> +			ranges = <0 0x01e00000 0xfff00000 0x100000>;
> +		};
> +	};

I think I'm a bit lost here. Is the "soc" node in this example the node
that is described as compatible="marvell,armada370-mbus"? Maybe expand
the example a little here to clarify this.

> +
> +In this case, the MBus driver will allocate a window with target ID = 0x01,
> +attribute ID = 0xe0, base address 0xfff00000, and size 0x100000.
> +The windowid cell encodes the target and attribute in the upper bytes.

If you use the new preprocessor feature we have in DT now, you can
actually list a macro that is used to assembly the cell from target
and attribute ID.

> +Note that, the above 'ranges' property is creating a fictitious 2-cell address
> +space with the windowid and the base address. We need to get rid of this
> +2-cell address space, translating it into a real address space consisting of
> +just the base address.

I would not describe it as "fictious" here, rather an as "defined". This
binding is what turns the fiction into a reality ;-)

> +The obvious way is to add a translation at the mbus-node level, like this:
> +
> +	soc {
> +		ranges = <0x01e00000 0xfff00000 0xfff00000 0x100000>;
> +		bootrom {
> +			ranges = <0 0x01e00000 0xfff00000 0x100000>;
> +		};
> +	};
> +
> +This would lead to a lot of 'ranges' properties duplication because
> +when you need to declare a new translation for a given device, you have
> +to do it at the per-board DTS file. But since 'ranges' entries are not
> +inherited from included dtsi files, you would have to go through each of the
> +included dtsi files, making sure you duplicate each of the previous entries.

The part about "inheriting" is not clear. Also, this is not written more
like a blog post than a specification ;-)

> +So to avoid this duplication, the MBus driver is capable of adding the
> +required translation entry to the device tree programatically. In other
> +words, when writing the device tree you don't need to extend the mbus-node
> +translation entry list; the driver will do it dynamically.

I think the key here is not to avoid duplication but to allow flexibility.
I would write this as:

The mappings in the mbus device are expected to be dynamically created by
the operating system at boot time, or a later point. Since the ranges property
in the mbus device must match the actual mappings, it is also expected
to be filled out dynamically. A boot loader may set up mappings before
loading the operating system and describe them in the ranges property.

> +So for instance, if you want to add support for a NOR device, that sits
> +behind the Device Bus controller, you would have this device tree:
> +(Note that the windowid cell is encoding the target ID = 0x01 and attribute
> +ID = 0x2f, and the selected base address for the window is 0xe8000000).
> +
> +	soc {
> +		devbus-bootcs {
> +			status = "okay";
> +			ranges = <0 0x012f0000 0xe8000000 0x8000000>;
> +

The example should also contain an #address-cells and #size-cells
property in the devbus-bootcs, as those are required for interpreting the
ranges property.

> +** About the special target ID and attribute ID
> +
> +As stated above, for each 'ranges' translation entry in mbus-node first-level
> +children, the MBus driver will allocate a decoding window.
> +However, in some cases it's desirable to declare a translation entry but have
> +the MBus driver skip it.

I would leave out here what the driver does, or say that the mbus driver "may"
skip them. For the purpose of this specification, it does not matter whether
the driver leaves the translation in place or changes it.

> +This is currently the case for the internal-regs and the pcie-controller
> +nodes.
> +
> +In the former, a dummy translation entry is needed to map the regular device
> +nodes into the {windowid / base address} address space, using a zeroed windowid
> +(target = 0x0, attribute = 0x0). The MBus driver will skip entries with such
> +windowid.
> +
> +	soc {
> +		/* ... */
> +		internal-regs {
> +			compatible = "simple-bus";
> +			ranges = <0 0 0 0x100000>;  /* Dummy translation */
> +			serial@12000 {
> +				reg = <0x12000 0x100>;
> +				/* ... */
> +			};
> +		};
> +	};

I don't understand the part about zeroing a windowid. Is "target = 0x0,
attribute = 0x0" the actual setting for the internal registers? If so,
don't call it a dummy translation or a special case.
If not, why not use the actual setting here?

> +In the pcie-controller case, we have two kinds of entries we need to skip.
> +The first one is identical to the internal-regs case; in fact it's declaring a
> +translation into the internal-regs address space.
> +
> +The second one is identified by a target ID = 0xff and attribute ID = 0xff.
> +We use this to create the mapping into the large region where the PCIe
> +controller will request decoding window allocations from the MBus driver.
> +
> +	soc {
> +		pcie-controller {
> +			ranges =
> +			       <0x82000000 0 0x40000    0          0x40000    0 0x00002000
> +				0x82000000 0 0x80000    0          0x80000    0 0x00002000
> +				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000
> +				0x81000000 0 0          0xffff0000 0xe8000000 0 0x00100000>;
> +
> +			pcie@1,0 {
> +				/* ... */
> +			};
> +		};
> +	};

Again, I don't understand this part. For the purpose of specification, I
would not make a special case here. It is not a hardware property that makes
these special but the way we use it from Linux. Consequently I would suggest
we skip the PCI ranges in the initial boot time setup by identifying the
pcie-controller node by its "compatible" property.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Fri, Jun 07, 2013 at 09:01:44PM +0200, Arnd Bergmann wrote:

> > +- compatible:	Should be set to one of the following:
> > +		marvell,armada370-mbus
> > +		marvell,armadaxp-mbus
> 
> I thought they are compatible with all older ones at the register level,
> as long as we describe all the differences in the ranges property
> and other properties.

Agree, maybe this can come later?
 
> > +- 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.
> 
> I think you should also list #address-cells=<2> and #size-cells=<1>
> as required here, as long as an optional "ranges" property.

Yes
 
> I think it makes sense to describe the "ranges" format here,
> even if we expect the property to be empty at boot time and
> only filled at run-time.

Yes, specifically describe how the 2 cell address format is mapped
into the marvell target ID form.

> > +	soc {
> > +		bootrom {
> > +			ranges = <0 0x01e00000 0xfff00000 0x100000>;
> > +		};
> > +	};
> 
> I think I'm a bit lost here. Is the "soc" node in this example the node
> that is described as compatible="marvell,armada370-mbus"? Maybe expand
> the example a little here to clarify this.

Agree, it should be the mbus node..
 
> If you use the new preprocessor feature we have in DT now, you can
> actually list a macro that is used to assembly the cell from target
> and attribute ID.

Yes, that would be great:

#define MAP_ID(target_id,attributes) (((target_id) << 24) | ((attributes) << 16)) 0
#define MAP_BOOTROM MAP_ID(0xXXXX,0xXXXX)

Such that MAP_BOOTROM expands to two cells

> > +	soc {
> > +		devbus-bootcs {
> > +			status = "okay";
> > +			ranges = <0 0x012f0000 0xe8000000 0x8000000>;
> 
> The example should also contain an #address-cells and #size-cells
> property in the devbus-bootcs, as those are required for interpreting the
> ranges property.

Right,

Is the ranges right though? I was expecting this:

 ranges = <0  0x012f0000 0  0x8000000>

The 2nd address cell in the 2dword space should almost always be 0.

The 2nd address cell should be interprited as the offset within the
target's window, not as some kind of physical base address.

>>+ (Note that the windowid cell is encoding the target ID = 0x01 and attribute
>>+ ID = 0x2f, and the selected base address for the window is 0xe8000000).

... The proper place to indicate the base address for the window is in
the mbus ranges:

mbus {
   ranges = <0x012f0000 0  0xe8000000  0x8000000>
   devbus-bootcs {
       ranges = <0  0x012f0000 0  0x8000000>
   }
}

We shouldn't mangle the DT format just to make it convenient for
humans to write - if this is a major problem then I'd try to use the
preprocessor first.. There are several reasonable solutions down that
path, IMHO.

> I don't understand the part about zeroing a windowid. Is "target = 0x0,
> attribute = 0x0" the actual setting for the internal registers? If so,
> don't call it a dummy translation or a special case.
> If not, why not use the actual setting here?

Internal regs has a special window register that doesn't have a target
id. We need to define an address cell for it, I suggest something like
<0xFFFFFFFF 0>, or <0x0000FFFF 0>...

> > +
> > +	soc {
> > +		pcie-controller {
> > +			ranges =
> > +			       <0x82000000 0 0x40000    0          0x40000    0 0x00002000
> > +				0x82000000 0 0x80000    0          0x80000    0 0x00002000
> > +				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000
> > +				0x81000000 0 0          0xffff0000 0xe8000000 0 0x00100000>;
> > +
> > +			pcie@1,0 {
> > +				/* ... */
> > +			};
> > +		};
> > +	};

> Again, I don't understand this part. For the purpose of specification, I
> would not make a special case here. It is not a hardware property that makes
> these special but the way we use it from Linux. Consequently I would suggest
> we skip the PCI ranges in the initial boot time setup by identifying the
> pcie-controller node by its "compatible" property.

This is tricky :| The PCIE controller needs both target IDs and the
aperture range to allocate them in.

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Friday 07 June 2013, Jason Gunthorpe wrote:
> On Fri, Jun 07, 2013 at 09:01:44PM +0200, Arnd Bergmann wrote:
> 
> > > +- compatible:	Should be set to one of the following:
> > > +		marvell,armada370-mbus
> > > +		marvell,armadaxp-mbus
> > 
> > I thought they are compatible with all older ones at the register level,
> > as long as we describe all the differences in the ranges property
> > and other properties.
> 
> Agree, maybe this can come later?

I'm just wondering if the name should be for the original device that
introduced them.

> > > +	soc {
> > > +		devbus-bootcs {
> > > +			status = "okay";
> > > +			ranges = <0 0x012f0000 0xe8000000 0x8000000>;
> > 
> > The example should also contain an #address-cells and #size-cells
> > property in the devbus-bootcs, as those are required for interpreting the
> > ranges property.
> 
> Right,
> 
> Is the ranges right though? I was expecting this:
> 
>  ranges = <0  0x012f0000 0  0x8000000>
> 
> The 2nd address cell in the 2dword space should almost always be 0.
> 
> The 2nd address cell should be interprited as the offset within the
> target's window, not as some kind of physical base address.

Ah, that's right. I missed that.

> >>+ (Note that the windowid cell is encoding the target ID = 0x01 and attribute
> >>+ ID = 0x2f, and the selected base address for the window is 0xe8000000).
> 
> ... The proper place to indicate the base address for the window is in
> the mbus ranges:
> 
> mbus {
>    ranges = <0x012f0000 0  0xe8000000  0x8000000>
>    devbus-bootcs {
>        ranges = <0  0x012f0000 0  0x8000000>
>    }
> }
> 
> We shouldn't mangle the DT format just to make it convenient for
> humans to write - if this is a major problem then I'd try to use the
> preprocessor first.. There are several reasonable solutions down that
> path, IMHO.

agreed.

> > I don't understand the part about zeroing a windowid. Is "target = 0x0,
> > attribute = 0x0" the actual setting for the internal registers? If so,
> > don't call it a dummy translation or a special case.
> > If not, why not use the actual setting here?
> 
> Internal regs has a special window register that doesn't have a target
> id. We need to define an address cell for it, I suggest something like
> <0xFFFFFFFF 0>, or <0x0000FFFF 0>...

Ok. I'd suggest using a value of '0' at least for the bits that are
used for encoding the other windows, and using the remaining bits to
identify whether it's the internal-reg or something else. One bit would
be enough for that, but 0xffff would work as well. I have a (weak)
preference for just using one bit. If we define a bit to mean "not
internal regs", then the macro could always set that, and zero would
actually be a valid and convenient representation of the internal regs.

> > > +
> > > +	soc {
> > > +		pcie-controller {
> > > +			ranges =
> > > +			       <0x82000000 0 0x40000    0          0x40000    0 0x00002000
> > > +				0x82000000 0 0x80000    0          0x80000    0 0x00002000
> > > +				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000
> > > +				0x81000000 0 0          0xffff0000 0xe8000000 0 0x00100000>;
> > > +
> > > +			pcie@1,0 {
> > > +				/* ... */
> > > +			};
> > > +		};
> > > +	};
> 
> > Again, I don't understand this part. For the purpose of specification, I
> > would not make a special case here. It is not a hardware property that makes
> > these special but the way we use it from Linux. Consequently I would suggest
> > we skip the PCI ranges in the initial boot time setup by identifying the
> > pcie-controller node by its "compatible" property.
> 
> This is tricky :| The PCIE controller needs both target IDs and the
> aperture range to allocate them in.

And the target ID is port specific, right?

Is the aperture range actually fixed in hardware in any way? Could we just
describe the entire 4GB translation for each port, and let the mbus driver
pass the aperture to the pcie driver, based on whatever address space is left
doing doing the boot-time window assignments?

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Hi Jason, Arnd:

Thanks for your reviews!

I agree with most of your suggestions so far. However, I'd like to discuss
one point before we move forward with the other (imo, less importants)
issues. See below.

On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
[...]
> 
> 
> Is the ranges right though? I was expecting this:
> 
>  ranges = <0  0x012f0000 0  0x8000000>
> 
> The 2nd address cell in the 2dword space should almost always be 0.
> 
> The 2nd address cell should be interprited as the offset within the
> target's window, not as some kind of physical base address.
> 
> >>+ (Note that the windowid cell is encoding the target ID = 0x01 and attribute
> >>+ ID = 0x2f, and the selected base address for the window is 0xe8000000).
> 
> ... The proper place to indicate the base address for the window is in
> the mbus ranges:
> 
> mbus {
>    ranges = <0x012f0000 0  0xe8000000  0x8000000>
>    devbus-bootcs {
>        ranges = <0  0x012f0000 0  0x8000000>
>    }
> }
> 
> We shouldn't mangle the DT format just to make it convenient for
> humans to write - if this is a major problem then I'd try to use the
> preprocessor first.. There are several reasonable solutions down that
> path, IMHO.
> 

Right. I think we have two options here for laying the DT ranges.

1) This is the proposal implied in the patchset I sent:

mbus {
	ranges = < we only put the internal-reg translation here>
	devbus-bootcs {
		ranges = <0 {target_id/attribute} {window_physical_base} {size}>
	}
}

Of course the above DT will be actually incomplete, for it'll lack a proper ranges
entry to translate the devbus-bootcs address. So we chosed to do it dynamically
in the mbus driver (see patch 05/14), and add the missing entry.

The information of the physical window base address is in this case in
each child (devbus-bootcs, bootrom, and so on). The MBus driver walks
each of its first-level children and allocates the window based on the
address declared in the ranges property of each child, as shown above.

This is done mostly to avoid having that in the mbus node, and the nightmare
to maintain it produces. See below.

2) This is what Jason is proposing in his mail:

mbus {
	ranges = <{target_id/attribute} 0 {window_physical_base} {size}>
	devbus-bootcs {
		ranges = <0 {target_id/attribute} 0 {size}>
	}
}

Of course this looks much cleaner, but it forces a lot of duplication
in the DT files. Now, if you see some of the recent patches we've been
sending, I think this duplication is very error-prone, and it'll be a
nightmare to maintain. Let me propose an example to show this
duplication:

Let's suppose we have a board "A" with its armada-A.dts,
and a common one armada.dtsi.

The common dtsi file would have this ranges property:

/* armada.dtsi */
mbus {
	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
		         bootrom_id 0       bootrom_base       bootrom_size >
}

The A board has a NOR connected to some devbus, so we need to add it
to the ranges, but also need to duplicate the ones in the common dtsi:

/* armada-A.dts */
mbus {
	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
		         bootrom_id 0       bootrom_base       bootrom_size
		         devbus0_id 0       devbus0_base       devbus0_size >
}

Now, if we add something at the common level, and extend the ranges
property in the common armada.dtsi, we also have to go through *each* of
the per-board dts files (for *each* board) adding that entry, because
entries *need* to be duplicated. Otherwise you're effectively
"shadowing" the entries.

It is precisely for this reason that I've decided to adopt option #1
instead! Now, I'm not saying I like that option particularly.
In fact it has a couple issues as well:

  1. The DT is *incomplete* and needs to be completed by the MBus
     driver which, IMHO, sucks.

  2. Changing the DT dynamically in the kernel, means that new
     properties are allocated to replace old ones, but the old ones
     are *never* released. So if for any reason we do this often,
     we're effectively "leaking" memory.

So, I'm not saying option #1 is nice, but rather that it seems it'll
be less trouble to maintain.

What do you think?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Sat, Jun 08, 2013 at 03:38:52PM -0300, Ezequiel Garcia wrote:

> > mbus {
> >    ranges = <0x012f0000 0  0xe8000000  0x8000000>
> >    devbus-bootcs {
> >        ranges = <0  0x012f0000 0  0x8000000>
> >    }
> > }
> > 
> > We shouldn't mangle the DT format just to make it convenient for
> > humans to write - if this is a major problem then I'd try to use the
> > preprocessor first.. There are several reasonable solutions down that
> > path, IMHO.
> > 
> 
> Right. I think we have two options here for laying the DT ranges.
> 
> 1) This is the proposal implied in the patchset I sent:
> 
> mbus {
> 	ranges = < we only put the internal-reg translation here>
> 	devbus-bootcs {
> 		ranges = <0 {target_id/attribute}
> 	{window_physical_base} {size}>
        ^^^^^^^^^^^^^^^^^^^^^^

This is the mangling I was referring to. It needs to be the offset
into the target, it can't be something else.

I understand your motivation, but this is not a good method of
encoding this in DT.

There is nothing especially wrong with updating the ranges at runtime,
but don't use the 2nd cell in the 2dw address to encode the desired
base address.

> Of course this looks much cleaner, but it forces a lot of duplication
> in the DT files. Now, if you see some of the recent patches we've been
> sending, I think this duplication is very error-prone, and it'll be a
> nightmare to maintain. Let me propose an example to show this
> duplication:

IMHO, dtc was not designed to do the sorts of include things that we
see in the boot/dt directory (eg a complex inheritance system) it is
not surprising we hit limitations in dtc.

It would be better to try and address them either by modding dtc (an
append directive of some kind), or using the preprocessor...

The preprocessor is already setup, here is a way to use it to solve
this problem:

 /* armada.dtsi */
 mbus {
 	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
 		         bootrom_id 0       bootrom_base  bootrom_size 
                   MBUS_BOARD_SPECIFIC_INFO>

 }
 
 /* armada-A.dts */
 define MBUS_BOARD_SPECIFIC_INFO devbus0_id 0       devbus0_base devbus0_size
 #include "armada.dtsi"

There are lots and lots of solutions using the pre-processor, please
take a look and find one that you feel works for you...

> It is precisely for this reason that I've decided to adopt option #1
> instead! Now, I'm not saying I like that option particularly.
> In fact it has a couple issues as well:

The main issue for me is that you can no longer refer to an offset in
a target, and that badly breaks several desirable DT layouts, eg on
Kirkwood the NAND case could be expressed like:

devbus {
  reg = <INTERNAL_REGS + 0x1234 0xABCD>;
  ranges = <0 DEVBUS_TARGET 0x1000>;
}

The NAND driver needs both internal regs and a target id in one
binding.

Which is why the last DW in the address must be the offset into the
target, not something else.

> What do you think?

IMHO, creating a correct FDT is of primary importance, the
maintainability of the text DT is secondary.

We can always improve the dtc envorionment (as was done recently by
adding the preprocessor), we cannot change 'DT ABI' once it it set.

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

On Sat, Jun 08, 2013 at 07:45:06PM -0600, Jason Gunthorpe wrote:
> On Sat, Jun 08, 2013 at 03:38:52PM -0300, Ezequiel Garcia wrote:
[...]
> 
> There are lots and lots of solutions using the pre-processor, please
> take a look and find one that you feel works for you...
> 

Ok, I will.

> 
> We can always improve the dtc envorionment (as was done recently by
> adding the preprocessor), we cannot change 'DT ABI' once it it set.
>

Right. Let me rework this using the preprocessor and send a v2.

Thanks for the feedback!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Jason, Arnd:

On Sat, Jun 08, 2013 at 07:45:06PM -0600, Jason Gunthorpe wrote:
[...]
> 
> This is the mangling I was referring to. It needs to be the offset
> into the target, it can't be something else.
> 

I see both of you have explained this already, but I can't seem
to catch it yet.

Mind explaining me exaclty *why* "it can't be something else"?
IOW, why the window base address cannot be in the 2nd cell,
and/or what use cases would this encoding brake?

> I understand your motivation, but this is not a good method of
> encoding this in DT.
> 
> There is nothing especially wrong with updating the ranges at runtime,
> but don't use the 2nd cell in the 2dw address to encode the desired
> base address.
> 

Regarding this, let me confirm your proposed layout using an example.
(later on, i'll try the preprocessor on this to make it look better).

mbus {
	compatible = "foo-mbus";
	ranges = < 0xffff0001 0 0xf1000000 0x1000000 /* Internal register entry */
		   0x011d0000 0 0xfff00000 0x0100000 /* BootROM entry with base address */
		   0x012f0000 0 0xe8000000 0x1000000 /* DevBus,NOR entry with base address */
	>;

	bootrom {
		ranges = <0 0x011d0000 0 0x100000>;
	};

	devbus-bootcs {
		ranges = <0 0x012f0000 0 0x1000000>;
		nor {
			reg = <0 0x1000000>;
		};
	};
};

Is the above correct?

Now, if the above is OK, then the DT looks complete to me. Meaning:

1. There's no need to update any ranges property at runtime.

2. There's no need to use an algorithm to 'find' a base address for decoding
   windows (such as first-fit as Arnd suggested in another mail).
   The base address is already there in the ranges property, so the mbus can
   simply allocate the window on that base address.

So, am I right or completely lost?

Thanks a lot for the help,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 11 June 2013 10:57:38 Ezequiel Garcia wrote:
> Jason, Arnd:
> 
> On Sat, Jun 08, 2013 at 07:45:06PM -0600, Jason Gunthorpe wrote:
> [...]
> > 
> > This is the mangling I was referring to. It needs to be the offset
> > into the target, it can't be something else.
> > 
> 
> I see both of you have explained this already, but I can't seem
> to catch it yet.
> 
> Mind explaining me exaclty *why* "it can't be something else"?
> IOW, why the window base address cannot be in the 2nd cell,
> and/or what use cases would this encoding brake?

Most importantly that would meant that the 'ranges' property
does not actually reflect the setting in the hardware mbus window,
which in fact remaps a base address of the upstream bus to address
zero of the downstream bus, and that address has to be offsettable.

Taking your example from below:

ranges = <0x011d0000 0 0xfff00000 0x0100000>; /* BootROM */

You map the start '0' of the boot rom to 0xfff0000. Your proposal
would mean setting up the intermediate address space to use the
same offset that is used on the host side, like 

ranges = <0x011d0000 0xfff00000 0xfff00000 0x0100000>;

This essentially you would have to set up the mbus window so the
bus address subtracts the second cell from the third cell to
get the child bus address, right?

One clear example where it would break down is when you have a
PCI bus window and e.g. map the PCI bus address 0xd0000000 to
host physical address 0xd0000000. Doing the same calculation
would mean you need a ranges property like

ranges = <0x0abc0000 0xa0000000 0xd0000000 0x01000000>;

so that when setting up the mbus you can subtract 0xd0000000
from 0xa0000000 and get back (wrapping around through 0) to
the address 0xd0000000 that you actually want. That would be
really stupid and illogical.

Finally, it makes setting up the ranges at boot time extremely
hard, unless you always want to map each device to the same
physical address (which is not always possible), because you
would have to not only change the ranges property of the mbus
node when you change the window, but also every single 'reg'
or 'ranges' property in the children referring to it.

> > I understand your motivation, but this is not a good method of
> > encoding this in DT.
> > 
> > There is nothing especially wrong with updating the ranges at runtime,
> > but don't use the 2nd cell in the 2dw address to encode the desired
> > base address.
> > 
> 
> Regarding this, let me confirm your proposed layout using an example.
> (later on, i'll try the preprocessor on this to make it look better).
> 
> mbus {
> 	compatible = "foo-mbus";
> 	ranges = < 0xffff0001 0 0xf1000000 0x1000000 /* Internal register entry */
> 		   0x011d0000 0 0xfff00000 0x0100000 /* BootROM entry with base address */
> 		   0x012f0000 0 0xe8000000 0x1000000 /* DevBus,NOR entry with base address */
> 	>;
> 
> 	bootrom {
> 		ranges = <0 0x011d0000 0 0x100000>;
> 	};
> 
> 	devbus-bootcs {
> 		ranges = <0 0x012f0000 0 0x1000000>;
> 		nor {
> 			reg = <0 0x1000000>;
> 		};
> 	};
> };
> 
> Is the above correct?

That looks ok to me, yes. If you have just one device under some of the nodes
however, I think it's easier use an empty ranges property and do

 	devbus-bootcs {
 		ranges;
 		nor {
 			reg = <0x012f0000 0 0x1000000>;
 		};

The 'ranges' here is most useful if you have multiple devices
add different offsets. I would also predefine those ranges to
be as large as possible so you don't have to adapt them when
the child device grows beyond it.

> Now, if the above is OK, then the DT looks complete to me. Meaning:
> 
> 1. There's no need to update any ranges property at runtime.

You still have to update the ranges every time you add, remove
or update a mapping window.

> 2. There's no need to use an algorithm to 'find' a base address for decoding
>    windows (such as first-fit as Arnd suggested in another mail).
>    The base address is already there in the ranges property, so the mbus can
>    simply allocate the window on that base address.
> 
> So, am I right or completely lost?

Since you already need code to update the ranges property, I think
it's best to discard the ranges at boot time and create a new mapping
that just maps the devices you actually want to use. That would make
it much easier to cope with boot loaders that don't set up the
mappings in the same way that is listed in the device tree. Just
put the one entry for the internal-regs in the ranges, since that
is needed to map do the mbus setup.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Tue, Jun 11, 2013 at 05:26:47PM +0200, Arnd Bergmann wrote:

> That looks ok to me, yes. If you have just one device under some of the nodes
> however, I think it's easier use an empty ranges property and do
> 
>  	devbus-bootcs {
>  		ranges;
>  		nor {
>  			reg = <0x012f0000 0 0x1000000>;
>  		};
> 
> The 'ranges' here is most useful if you have multiple devices
> add different offsets. I would also predefine those ranges to
> be as large as possible so you don't have to adapt them when
> the child device grows beyond it.

It isn't super critical, but the ranges does keep the 0x012f0000 value
in the SOC .dtsi and the board level doesn't have to be exposed to the
value, it just uses 0 to setup the NOR.

Also, it makes it much simpler for the mbus driver to detect which
target id's are required so they can be allocated/setup - the rule
would be every child stanza should have a ranges with the target(s) it
needs.

The full version probably looks like this:

mbus {
    ranges ....

                        devbus-bootcs@10400 {
                                compatible = "marvell,mvebu-devbus";
                                reg = <INTERNAL_REGS + 0x10400 0x8>;
                                ranges = <0 DEVBUS_BOOTCS 0x1000000>;
                                #address-cells = <1>;
                                #size-cells = <1>;
                                clocks = <&coreclk 0>;

				// In board .dts:
                                nor@0 {
                                        compatible = "cfi-flash";
                                        reg = <0 0x1000000>;
                                        bank-width = <2>;
                                };
                        };
};

And notice here that we need to offset INTERNAL_REGS to make this
work. To me, this is the primary reason why the 2nd cell of the mbus
address format *must* be the address offset within the target, and not
anything else.

> > 2. There's no need to use an algorithm to 'find' a base address for decoding
> >    windows (such as first-fit as Arnd suggested in another mail).
> >    The base address is already there in the ranges property, so the mbus can
> >    simply allocate the window on that base address.
> > 
> > So, am I right or completely lost?
> 
> Since you already need code to update the ranges property, I think
> it's best to discard the ranges at boot time and create a new mapping
> that just maps the devices you actually want to use. That would make
> it much easier to cope with boot loaders that don't set up the
> mappings in the same way that is listed in the device tree. Just
> put the one entry for the internal-regs in the ranges, since that
> is needed to map do the mbus setup.

So, I think we can/have agree that the ranges in the FDT should
reflect the bootloader's settings, and if the ranges is missing an
element it means the bootloader didn't set it up.

A compatible bootloader should dump the entire mbus register table
into the ranges. The address encoding is such that we can represent
every mbus window entry in ranges. If no bootloader support is present
then the ranges value is forced into the mbus windows anyhow.

If there isn't a ranges entry, but there is a child that requires it
(how do we figure this out?) then dynamic assignment of that window
makes sense to me. (however, this also looks a bit tricky, how do you
avoid hitting the PCI-E window reservations, for instance?)

Unconditional dynamic assignment is a bit more tricky, for instance
the boot rom has to be located at a specific address since it is used
as the SMP secondary startup trampoline. (Ezequiel: IMHO, you should
check for this and bail secondary startup if it is not setup)

I'm not sure there is a good reason to reject the address map in the
DT?

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Sebastian Hesselbarth]

On 06/11/2013 11:50 PM, Jason Gunthorpe wrote:
> On Tue, Jun 11, 2013 at 05:26:47PM +0200, Arnd Bergmann wrote:
>> That looks ok to me, yes. If you have just one device under some of the nodes
>> however, I think it's easier use an empty ranges property and do
>>
>>   	devbus-bootcs {
>>   		ranges;
>>   		nor {
>>   			reg =<0x012f0000 0 0x1000000>;
>>   		};
>>
>> The 'ranges' here is most useful if you have multiple devices
>> add different offsets. I would also predefine those ranges to
>> be as large as possible so you don't have to adapt them when
>> the child device grows beyond it.
>
> It isn't super critical, but the ranges does keep the 0x012f0000 value
> in the SOC .dtsi and the board level doesn't have to be exposed to the
> value, it just uses 0 to setup the NOR.

Jason, Ezequiel, Arnd,

sorry to kick into this thread that late but Ezequiel made me think of
address windows when asking on IRC. From what I can see from your and
Arnd's proposal the only real difference is that having it Arnd's way
allows you to have two or more windows mapped to e.g. devbus with
totally independent address ranges - while yours requires one large
window for all child devices?

> Also, it makes it much simpler for the mbus driver to detect which
> target id's are required so they can be allocated/setup - the rule
> would be every child stanza should have a ranges with the target(s) it
> needs.
>
> The full version probably looks like this:
>
> mbus {
>      ranges ....
>
>                          devbus-bootcs@10400 {
>                                  compatible = "marvell,mvebu-devbus";
>                                  reg =<INTERNAL_REGS + 0x10400 0x8>;
>                                  ranges =<0 DEVBUS_BOOTCS 0x1000000>;
>                                  #address-cells =<1>;
>                                  #size-cells =<1>;
>                                  clocks =<&coreclk 0>;
>
> 				// In board .dts:
>                                  nor@0 {
>                                          compatible = "cfi-flash";
>                                          reg =<0 0x1000000>;
>                                          bank-width =<2>;
>                                  };
>                          };
> };
>
> And notice here that we need to offset INTERNAL_REGS to make this
> work. To me, this is the primary reason why the 2nd cell of the mbus
> address format *must* be the address offset within the target, and not
> anything else.

Given the limited number of windows, I personally prefer your proposal
above - but nevertheless I can also think of separating target id from
ranges completely. (I know this discussion has been here for a while,
and I apologize if below was already NACKed before)

Although, not thought to the bare end this could also work and maybe
reflects the actual hardware more accuate:

mbus {
	compatible = "marvell,kirkwood-mbus";
	ranges; /* 1:1 mapping */

	internal-regs {
		ranges = <0 0xf1000000 0x800000>;
		/* target-id target-attr size */
		marvell,mbus-target = <0xff 0x12 0x800000>;

		nand {
			compatible = "marvell,orion-nand";
			reg = <0x10400 0x80>;
			marvell,mbus-target = <0x01 0x2f 0x100000>;

			/* in board dts */
			spansion-flash {
				...
			};
		};
	};

	sram {
		marvell,mbus-target = <...>;
	};
};

>>> 2. There's no need to use an algorithm to 'find' a base address for decoding
>>>     windows (such as first-fit as Arnd suggested in another mail).
>>>     The base address is already there in the ranges property, so the mbus can
>>>     simply allocate the window on that base address.
>>>
>>> So, am I right or completely lost?
>>
>> Since you already need code to update the ranges property, I think
>> it's best to discard the ranges at boot time and create a new mapping
>> that just maps the devices you actually want to use. That would make
>> it much easier to cope with boot loaders that don't set up the
>> mappings in the same way that is listed in the device tree. Just
>> put the one entry for the internal-regs in the ranges, since that
>> is needed to map do the mbus setup.
>
> So, I think we can/have agree that the ranges in the FDT should
> reflect the bootloader's settings, and if the ranges is missing an
> element it means the bootloader didn't set it up.
>
> A compatible bootloader should dump the entire mbus register table
> into the ranges. The address encoding is such that we can represent
> every mbus window entry in ranges. If no bootloader support is present
> then the ranges value is forced into the mbus windows anyhow.
>
> If there isn't a ranges entry, but there is a child that requires it
> (how do we figure this out?) then dynamic assignment of that window
> makes sense to me. (however, this also looks a bit tricky, how do you
> avoid hitting the PCI-E window reservations, for instance?)
>
> Unconditional dynamic assignment is a bit more tricky, for instance
> the boot rom has to be located at a specific address since it is used
> as the SMP secondary startup trampoline. (Ezequiel: IMHO, you should
> check for this and bail secondary startup if it is not setup)

When removing size from marvell,mbus-target above, mbus driver could
also probe for required max size from the reg properties of the child
nodes. Arnd already said that mbus isn't "simple-bus" anymore, why
can't it just walk through the nodes and collect required windows?

Sebastian

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 12 June 2013 00:22:29 Sebastian Hesselbarth wrote:
> 
> When removing size from marvell,mbus-target above, mbus driver could
> also probe for required max size from the reg properties of the child
> nodes. Arnd already said that mbus isn't "simple-bus" anymore, why
> can't it just walk through the nodes and collect required windows?

It would work, but feels wrong to me. The 'reg' properties of each
device behind mbus would end up as software defined and need to
be updated for any change to the mapping, whether that change is
board specific or even at run-time. It would be even harder to
do dynamic assignment when you cannot put a proper address into
'reg'.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Wed, Jun 12, 2013 at 12:22:29AM +0200, Sebastian Hesselbarth wrote:

> sorry to kick into this thread that late but Ezequiel made me think of
> address windows when asking on IRC. From what I can see from your and
> Arnd's proposal the only real difference is that having it Arnd's way
> allows you to have two or more windows mapped to e.g. devbus with
> totally independent address ranges - while yours requires one large
> window for all child devices?

Sorry, I'm having problems following the possessives in your paragraph
- which things are you comparing?

> Although, not thought to the bare end this could also work and maybe
> reflects the actual hardware more accuate:

I actually posted something like this a time ago, but Arnd's
suggestion to encode the target id in the address seems much clearer,
and more accurate, though a bit more work to parse.

It is 'better' in that the base addresses are in exactly one place -
the ranges of the mbus node. With marvell,mbus-target the base
addreses are strewn everywhere and now are difficult to change.

A main consideration here is that the bootloader needs to update the
base addresses - to support the 0xd0 vs 0xf1 internal-regs base
address issue. So I think that requirement alone is enough to make
marvell,mbus-target undesirable.

> When removing size from marvell,mbus-target above, mbus driver could
> also probe for required max size from the reg properties of the child
> nodes. Arnd already said that mbus isn't "simple-bus" anymore, why
> can't it just walk through the nodes and collect required windows?

With enough restrictions, I think it is pretty easy to parse
marvell,mbus-target, but you can employ the same restrictions to
simplify parsing the 2 cell address format too.

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Sebastian Hesselbarth]

On 06/12/2013 01:08 AM, Jason Gunthorpe wrote:
> On Wed, Jun 12, 2013 at 12:22:29AM +0200, Sebastian Hesselbarth wrote:
>> sorry to kick into this thread that late but Ezequiel made me think of
>> address windows when asking on IRC. From what I can see from your and
>> Arnd's proposal the only real difference is that having it Arnd's way
>> allows you to have two or more windows mapped to e.g. devbus with
>> totally independent address ranges - while yours requires one large
>> window for all child devices?
>
> Sorry, I'm having problems following the possessives in your paragraph
> - which things are you comparing?

Ahh, just forget what I said. I see you two wrapped you mind a dozen
times more around it. I am starting to get the idea and its "beauty" ;)

Sebastian

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 11 June 2013 15:50:23 Jason Gunthorpe wrote:
> On Tue, Jun 11, 2013 at 05:26:47PM +0200, Arnd Bergmann wrote:
> 
> > That looks ok to me, yes. If you have just one device under some of the nodes
> > however, I think it's easier use an empty ranges property and do
> > 
> >  	devbus-bootcs {
> >  		ranges;
> >  		nor {
> >  			reg = <0x012f0000 0 0x1000000>;
> >  		};
> > 
> > The 'ranges' here is most useful if you have multiple devices
> > add different offsets. I would also predefine those ranges to
> > be as large as possible so you don't have to adapt them when
> > the child device grows beyond it.
> 
> It isn't super critical, but the ranges does keep the 0x012f0000 value
> in the SOC .dtsi and the board level doesn't have to be exposed to the
> value, it just uses 0 to setup the NOR.

Yes, good point.

> Also, it makes it much simpler for the mbus driver to detect which
> target id's are required so they can be allocated/setup - the rule
> would be every child stanza should have a ranges with the target(s) it
> needs.

Not sure if this is a good rule: As you said, it makes sense to
put the 'ranges' for the devbus-bootcs node into the per-soc .dtsi
file so the board file doesn't have to care. However the mbus code
needs both the 0x012f0000 and the start/length of the child. Mapping
the entire window would work in theory but is likely going to cause
a significant waste of physical address space, because the (per-soc)
ranges property has to be set up for the largest possible external
device connected to the bus, but the mbus window only needs to cover the
device that is actually connected.

> The full version probably looks like this:
> 
> mbus {
>     ranges ....
> 
>                         devbus-bootcs@10400 {
>                                 compatible = "marvell,mvebu-devbus";
>                                 reg = <INTERNAL_REGS + 0x10400 0x8>;
>                                 ranges = <0 DEVBUS_BOOTCS 0x1000000>;
>                                 #address-cells = <1>;
>                                 #size-cells = <1>;
>                                 clocks = <&coreclk 0>;
> 
> 				// In board .dts:
>                                 nor@0 {
>                                         compatible = "cfi-flash";
>                                         reg = <0 0x1000000>;
>                                         bank-width = <2>;
>                                 };
>                         };
> };
> 
> And notice here that we need to offset INTERNAL_REGS to make this
> work. To me, this is the primary reason why the 2nd cell of the mbus
> address format *must* be the address offset within the target, and not
> anything else.

Right.

> > > 2. There's no need to use an algorithm to 'find' a base address for decoding
> > >    windows (such as first-fit as Arnd suggested in another mail).
> > >    The base address is already there in the ranges property, so the mbus can
> > >    simply allocate the window on that base address.
> > > 
> > > So, am I right or completely lost?
> > 
> > Since you already need code to update the ranges property, I think
> > it's best to discard the ranges at boot time and create a new mapping
> > that just maps the devices you actually want to use. That would make
> > it much easier to cope with boot loaders that don't set up the
> > mappings in the same way that is listed in the device tree. Just
> > put the one entry for the internal-regs in the ranges, since that
> > is needed to map do the mbus setup.
> 
> So, I think we can/have agree that the ranges in the FDT should
> reflect the bootloader's settings, and if the ranges is missing an
> element it means the bootloader didn't set it up.
> 
> A compatible bootloader should dump the entire mbus register table
> into the ranges. The address encoding is such that we can represent
> every mbus window entry in ranges. If no bootloader support is present
> then the ranges value is forced into the mbus windows anyhow.
> 
> If there isn't a ranges entry, but there is a child that requires it
> (how do we figure this out?)

I think we have to walk the entire tree of devices underneath mbus,
at least any device node that has a 'reg' property, following children
of any device node with a 'ranges' property. We might need to
add a variant of of_get_address() that does a partial translation
up to a given node (the mbus) instead of all the way to the root.

> then dynamic assignment of that window
> makes sense to me. (however, this also looks a bit tricky, how do you
> avoid hitting the PCI-E window reservations, for instance?)

The PCI-E window would have 'ranges' but no 'regs', so we automatically
skip it.

> Unconditional dynamic assignment is a bit more tricky, for instance
> the boot rom has to be located at a specific address since it is used
> as the SMP secondary startup trampoline. (Ezequiel: IMHO, you should
> check for this and bail secondary startup if it is not setup)

I suppose we can identify the devices we don't want to reassign by
their compatible values.

> I'm not sure there is a good reason to reject the address map in the
> DT?

You mean keep all windows that are listed by the boot loader but
reassign the others? I guess it would be simpler but may result in
a less optimal address map.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Wed, Jun 12, 2013 at 12:34:14AM +0200, Arnd Bergmann wrote:

> a significant waste of physical address space, because the (per-soc)
> ranges property has to be set up for the largest possible external
> device connected to the bus, but the mbus window only needs to cover the
> device that is actually connected.

Yes, but perhaps that is a good argument to not put the ranges in the
dtsi?
 
> I think we have to walk the entire tree of devices underneath mbus,
> at least any device node that has a 'reg' property, following children
> of any device node with a 'ranges' property. We might need to
> add a variant of of_get_address() that does a partial translation
> up to a given node (the mbus) instead of all the way to the root.

Hum, how complex do you think this is? Run down the entire tree,
translate every regs and maintain a max offset for every target id.

> > then dynamic assignment of that window
> > makes sense to me. (however, this also looks a bit tricky, how do you
> > avoid hitting the PCI-E window reservations, for instance?)
> 
> The PCI-E window would have 'ranges' but no 'regs', so we automatically
> skip it.

The trouble is not skipping PCI-E when parsing the dt. When the
kernel goes to make a dynamic window allocation it needs to exclude
the address space reserved for the PCI-E aperture.

The mbus driver starts before the PCI-E driver, so how can this be
resolved? Hardwire specialness about PCI-E into mbus?
Make the PCI-E aperture fully dynamic?

Starting to be pretty complex - for what gain?

> > I'm not sure there is a good reason to reject the address map in the
> > DT?
> 
> You mean keep all windows that are listed by the boot loader but
> reassign the others? I guess it would be simpler but may result in
> a less optimal address map.

Sure, but what does a more optimal address map get you?

 1) More space to map SDRAM - however - the SDRAM map is controlled
    exclusively by the bootloader and the kernel doesn't have the
    information or the code to mess with it - so this isn't possible
 2) More space for the PCI-E aperture - this is entirely controlled by
    the kernel, but today we are using the pre-set allocation from the
    DT, so it doesn't matter how tightly the rest of the stuff is packed.

To me, it feels like alot of complex trouble to do actual dynamic
allocation, and I don't really see any gain.

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 11 June 2013 16:58:41 Jason Gunthorpe wrote:
> On Wed, Jun 12, 2013 at 12:34:14AM +0200, Arnd Bergmann wrote:
> 
> > a significant waste of physical address space, because the (per-soc)
> > ranges property has to be set up for the largest possible external
> > device connected to the bus, but the mbus window only needs to cover the
> > device that is actually connected.
> 
> Yes, but perhaps that is a good argument to not put the ranges in the
> dtsi?

Fair enough. In that case not using ranges at all is probably easier
so the board file only needs to update one place.

> > I think we have to walk the entire tree of devices underneath mbus,
> > at least any device node that has a 'reg' property, following children
> > of any device node with a 'ranges' property. We might need to
> > add a variant of of_get_address() that does a partial translation
> > up to a given node (the mbus) instead of all the way to the root.
> 
> Hum, how complex do you think this is? Run down the entire tree,
> translate every regs and maintain a max offset for every target id.

It's the same thing we do for PCI.

> > > then dynamic assignment of that window
> > > makes sense to me. (however, this also looks a bit tricky, how do you
> > > avoid hitting the PCI-E window reservations, for instance?)
> > 
> > The PCI-E window would have 'ranges' but no 'regs', so we automatically
> > skip it.
> 
> The trouble is not skipping PCI-E when parsing the dt. When the
> kernel goes to make a dynamic window allocation it needs to exclude
> the address space reserved for the PCI-E aperture.
> 
> The mbus driver starts before the PCI-E driver, so how can this be
> resolved? Hardwire specialness about PCI-E into mbus?
> Make the PCI-E aperture fully dynamic?
>
> Starting to be pretty complex - for what gain?

I think this can be simplified a lot by making PCI a known special case:

We assign all regular (non-PCI) windows from the top, and whatever is
left by the time PCI comes up can be used for that, starting at the
bottom of the avaliable space.
 
> > > I'm not sure there is a good reason to reject the address map in the
> > > DT?
> > 
> > You mean keep all windows that are listed by the boot loader but
> > reassign the others? I guess it would be simpler but may result in
> > a less optimal address map.
> 
> Sure, but what does a more optimal address map get you?
> 
>  1) More space to map SDRAM - however - the SDRAM map is controlled
>     exclusively by the bootloader and the kernel doesn't have the
>     information or the code to mess with it - so this isn't possible
>  2) More space for the PCI-E aperture - this is entirely controlled by
>     the kernel, but today we are using the pre-set allocation from the
>     DT, so it doesn't matter how tightly the rest of the stuff is packed.
> 
> To me, it feels like alot of complex trouble to do actual dynamic
> allocation, and I don't really see any gain.

We could of course decide to skip the dynamic part in the Linux
implementation for the moment, but I would definitely want to
see the binding written in a way that going fully dynamic can be
done later without changing the binding if we decide we need to
squeeze out more RAM or PCI space.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Grant Likely]

On Wed, 12 Jun 2013 00:34:14 +0200, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Tuesday 11 June 2013 15:50:23 Jason Gunthorpe wrote:
> > So, I think we can/have agree that the ranges in the FDT should
> > reflect the bootloader's settings, and if the ranges is missing an
> > element it means the bootloader didn't set it up.
> > 
> > A compatible bootloader should dump the entire mbus register table
> > into the ranges. The address encoding is such that we can represent
> > every mbus window entry in ranges. If no bootloader support is present
> > then the ranges value is forced into the mbus windows anyhow.
> > 
> > If there isn't a ranges entry, but there is a child that requires it
> > (how do we figure this out?)
> 
> I think we have to walk the entire tree of devices underneath mbus,
> at least any device node that has a 'reg' property, following children
> of any device node with a 'ranges' property. We might need to
> add a variant of of_get_address() that does a partial translation
> up to a given node (the mbus) instead of all the way to the root.

I think we should have almost everything needed already to do this.
of_bus allows arbitrary mapping code to be used at any stage in the
translation. We might need to add some glue so that of_busses[] can
be assembled by the linker to allow an mbus of_bus stanza to live
outside of drivers/of/address.c

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 12 June 2013, Grant Likely wrote:
> I think we should have almost everything needed already to do this.
> of_bus allows arbitrary mapping code to be used at any stage in the
> translation. We might need to add some glue so that of_busses[] can
> be assembled by the linker to allow an mbus of_bus stanza to live
> outside of drivers/of/address.c
> 

Actually, the best thing about this solution is that we don't even
have to bother setting up the mappings when loading the mbus driver:
We don't need any ranges (other than internal-regs) in DT, and we
don't need complex logic to search through the child devices to
figure out what the mappings should be. The only thing one needs
to do here is check if a mapping already exists when we get into the
of_bus handler and create one for the device being translated if
there isn't one!

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

On Wed, Jun 12, 2013 at 10:45:55PM +0200, Arnd Bergmann wrote:
> On Wednesday 12 June 2013, Grant Likely wrote:
> > I think we should have almost everything needed already to do this.
> > of_bus allows arbitrary mapping code to be used at any stage in the
> > translation. We might need to add some glue so that of_busses[] can
> > be assembled by the linker to allow an mbus of_bus stanza to live
> > outside of drivers/of/address.c
> > 
> 
> Actually, the best thing about this solution is that we don't even
> have to bother setting up the mappings when loading the mbus driver:
> We don't need any ranges (other than internal-regs) in DT, and we
> don't need complex logic to search through the child devices to
> figure out what the mappings should be. The only thing one needs
> to do here is check if a mapping already exists when we get into the
> of_bus handler and create one for the device being translated if
> there isn't one!
> 

This departs considerably from what I'm aiming right now.

Are you suggesting to not put *any* mapping in the mbus 'ranges' node in
the DT (other than internal-regs)?

In that case, can you explain a bit further how the mbus would
decide a base address for each window? I guess you're thinking
in a first-fit algorithm, right?

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Wed, Jun 12, 2013 at 06:12:22PM -0300, Ezequiel Garcia wrote:

> > Actually, the best thing about this solution is that we don't even
> > have to bother setting up the mappings when loading the mbus driver:
> > We don't need any ranges (other than internal-regs) in DT, and we
> > don't need complex logic to search through the child devices to
> > figure out what the mappings should be. The only thing one needs

To get optimal packing you need to have the list in advance so it can
be sorted and best fit.

Further, remember that to allocate the window we need to learn the
maximum end offset of *all* reg users for that target id, not just the
first one to request translation. Not sure how the of_bus stuff gets
that for you??

> > to do here is check if a mapping already exists when we get into the
> > of_bus handler and create one for the device being translated if
> > there isn't one!
> 
> This departs considerably from what I'm aiming right now.
> 
> Are you suggesting to not put *any* mapping in the mbus 'ranges' node in
> the DT (other than internal-regs)?

My view is that the ranges should be respected. If an address is
present, it should be used, unless there is a conflict.

If no address is present (unclear why anyone would do this, but OK)
then demand allocate via the linux resource allocator through the
of_bus stuff seems reasonable to me..

Just to re-iterate Arnd's earlier comment: The DT representation
must handle dynamic allocation, but we can defer implementing the
kernel side until there is a need.

It isn't clear to me there is a need.....

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

On Wed, Jun 12, 2013 at 03:26:42PM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 12, 2013 at 06:12:22PM -0300, Ezequiel Garcia wrote:
> 
[...]
> > 
> > This departs considerably from what I'm aiming right now.
> > 
> > Are you suggesting to not put *any* mapping in the mbus 'ranges' node in
> > the DT (other than internal-regs)?
> 
> My view is that the ranges should be respected. If an address is
> present, it should be used, unless there is a conflict.
> 
> If no address is present (unclear why anyone would do this, but OK)
> then demand allocate via the linux resource allocator through the
> of_bus stuff seems reasonable to me..
> 
> Just to re-iterate Arnd's earlier comment: The DT representation
> must handle dynamic allocation, but we can defer implementing the
> kernel side until there is a need.
> 
> It isn't clear to me there is a need.....
> 

Right. And just to confirm: this kernel side dynamic implementation
will be completely independent of the MBus DT layout proposal.
So I think it's best to agree on this binding first.

I'll post a v2 with the progress I've made using the preprocessor.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 12 June 2013, Ezequiel Garcia wrote:
> Right. And just to confirm: this kernel side dynamic implementation
> will be completely independent of the MBus DT layout proposal.
> So I think it's best to agree on this binding first.
> 
> I'll post a v2 with the progress I've made using the preprocessor.

Right. My thought was just that with the of_bus handler it might end up
being /easier/ to do the dynamic mapping than it is to preconfigure
it in the DT.

Whether that results in an optimum mapping or not depends on the actual
sizes for the child nodes. Since everything is a power-of-two size,
a first-fit algorithm actually isn't bad at all.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Wed, Jun 12, 2013 at 11:52:32PM +0200, Arnd Bergmann wrote:
> Whether that results in an optimum mapping or not depends on the actual
> sizes for the child nodes. Since everything is a power-of-two size,
> a first-fit algorithm actually isn't bad at all.

The windows must be aligned to their size. eg 1M size window must be
aligned to a 1M boundary.

First fit only works if you allocate from largest alignment
requirement to smallest, otherwise the worst case is something like
nearly double the largest alignment wasted in alignment padding.

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Thursday 13 June 2013, Jason Gunthorpe wrote:
> On Wed, Jun 12, 2013 at 11:52:32PM +0200, Arnd Bergmann wrote:
> > Whether that results in an optimum mapping or not depends on the actual
> > sizes for the child nodes. Since everything is a power-of-two size,
> > a first-fit algorithm actually isn't bad at all.
> 
> The windows must be aligned to their size. eg 1M size window must be
> aligned to a 1M boundary.
> 
> First fit only works if you allocate from largest alignment
> requirement to smallest, otherwise the worst case is something like
> nearly double the largest alignment wasted in alignment padding.

Yes, I realize that, which is why I said it depends on the actual sizes.
If you have a lot of windows that are all the same size and nothing larger,
the worst case isn't too bad.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Thursday 13 June 2013, Arnd Bergmann wrote:

> Yes, I realize that, which is why I said it depends on the actual sizes.
> If you have a lot of windows that are all the same size and nothing larger,
> the worst case isn't too bad.

Also, if the available space is a naturally aligned address range with a
power-of-two size itself, the order does not matter at all and any order
is as good as any other.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Grant Likely]

On Wed, 12 Jun 2013 15:26:42 -0600, Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Wed, Jun 12, 2013 at 06:12:22PM -0300, Ezequiel Garcia wrote:
> 
> > > Actually, the best thing about this solution is that we don't even
> > > have to bother setting up the mappings when loading the mbus driver:
> > > We don't need any ranges (other than internal-regs) in DT, and we
> > > don't need complex logic to search through the child devices to
> > > figure out what the mappings should be. The only thing one needs
> 
> To get optimal packing you need to have the list in advance so it can
> be sorted and best fit.
> 
> Further, remember that to allocate the window we need to learn the
> maximum end offset of *all* reg users for that target id, not just the
> first one to request translation. Not sure how the of_bus stuff gets
> that for you??
> 
> > > to do here is check if a mapping already exists when we get into the
> > > of_bus handler and create one for the device being translated if
> > > there isn't one!
> > 
> > This departs considerably from what I'm aiming right now.
> > 
> > Are you suggesting to not put *any* mapping in the mbus 'ranges' node in
> > the DT (other than internal-regs)?
> 
> My view is that the ranges should be respected. If an address is
> present, it should be used, unless there is a conflict.
> 
> If no address is present (unclear why anyone would do this, but OK)
> then demand allocate via the linux resource allocator through the
> of_bus stuff seems reasonable to me..

Care is needed here however. As far as complexity; I would score it thus
(where lower numbers mean lower complexity):

1:   Fully described in DT; no dynamic resolution
5:   Fully /unresolved/ in DT; everything must be dynamically resolved
100: Mixed mode; some stuff fully resolved, others dynamically resolved.

The moment you start to do mixed mode, the whole thing gets really hairy
and difficult to get right. I you do proceed down this path, then it
would be a really good idea to put together a generic resolved library
that knows how to find and resolve all the static mappings before
resolving dynamic ones. Then write test cases for the thing so we can
know that it actually does what we want it to do.

> Just to re-iterate Arnd's earlier comment: The DT representation
> must handle dynamic allocation, but we can defer implementing the
> kernel side until there is a need.
> 
> It isn't clear to me there is a need.....

I also am nervous about depending on dynamic resolution on something
that isn't a dynamic bus... [although we're probably going to get pushed
in this direction anyway when support for device tree overlays gets
merged]

g.

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Jason,

On Tue, Jun 11, 2013 at 03:50:23PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 11, 2013 at 05:26:47PM +0200, Arnd Bergmann wrote:
> 
> > That looks ok to me, yes. If you have just one device under some of the nodes
> > however, I think it's easier use an empty ranges property and do
> > 
> >  	devbus-bootcs {
> >  		ranges;
> >  		nor {
> >  			reg = <0x012f0000 0 0x1000000>;
> >  		};
> > 
> > The 'ranges' here is most useful if you have multiple devices
> > add different offsets. I would also predefine those ranges to
> > be as large as possible so you don't have to adapt them when
> > the child device grows beyond it.
> 
> It isn't super critical, but the ranges does keep the 0x012f0000 value
> in the SOC .dtsi and the board level doesn't have to be exposed to the
> value, it just uses 0 to setup the NOR.
> 
> Also, it makes it much simpler for the mbus driver to detect which
> target id's are required so they can be allocated/setup - the rule
> would be every child stanza should have a ranges with the target(s) it
> needs.
> 
> The full version probably looks like this:
> 
> mbus {
>     ranges ....
> 
>                         devbus-bootcs@10400 {
>                                 compatible = "marvell,mvebu-devbus";
>                                 reg = <INTERNAL_REGS + 0x10400 0x8>;
>                                 ranges = <0 DEVBUS_BOOTCS 0x1000000>;
>                                 #address-cells = <1>;
>                                 #size-cells = <1>;
>                                 clocks = <&coreclk 0>;
> 
> 				// In board .dts:
>                                 nor@0 {
>                                         compatible = "cfi-flash";
>                                         reg = <0 0x1000000>;
>                                         bank-width = <2>;
>                                 };
>                         };
> };
> 
> And notice here that we need to offset INTERNAL_REGS to make this
> work. To me, this is the primary reason why the 2nd cell of the mbus
> address format *must* be the address offset within the target, and not
> anything else.
> 

I tried to implement something like the above, but adding numbers
does not seem to work using the preprocessor on the DT.

I'm probably missing something, but... are you sure that's supposed to work?

I ended up implementing something similar, see below.
The defines shown are very early/hacky, just to get it working.
I'll implement them better, so omit them for now.

#define INTERNAL_REGS_ID 0xffff0001

#define INTERNAL_REGS INTERNAL_REGS_ID 0
#define DEVBUS_BOOTCS 0x012f0000 0
#define BOOTROM       0x01e00000 0

#define INTERNAL_REGS_BASE 0 0xd0000000
#define DEVBUS_BOOTCS_BASE 0 0xf0000000
#define BOOTROM_BASE       0 0xfff00000
...
 
soc {
	ranges = <INTERNAL_REGS INTERNAL_REGS_BASE 0x100000
		  BOOTROM       BOOTROM_BASE       0x100000
		  DEVBUS_BOOTCS DEVBUS_BOOTCS_BASE 0x1000000>;

	devbus-bootcs {
		compatible = "marvell,mvebu-devbus";
		reg = <INTERNAL_REGS_ID 0x10400 0x8>;
		ranges = <0 DEVBUS_BOOTCS 0x1000000>;
		#address-cells = <1>;
		#size-cells = <1>;
		status = "disabled";
		
		nor@0 {
			compatible = "cfi-flash";
			reg = <0 0x1000000>;
			bank-width = <2>;
		};
	};
};

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Jason Gunthorpe]

On Wed, Jun 12, 2013 at 05:02:50PM -0300, Ezequiel Garcia wrote:

> I'm probably missing something, but... are you sure that's supposed to work?

Hum.

Looking through the yacc for dtc, it looks like all exprs must be enclosed
in (), so no it shouldn't work, sadly.

Jason

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 12 June 2013, Ezequiel Garcia wrote:
> #define INTERNAL_REGS INTERNAL_REGS_ID 0
> #define DEVBUS_BOOTCS 0x012f0000 0
> #define BOOTROM       0x01e00000 0
> 
> #define INTERNAL_REGS_BASE 0 0xd0000000
> #define DEVBUS_BOOTCS_BASE 0 0xf0000000
> #define BOOTROM_BASE       0 0xfff00000
> ...
>  
> soc {
>         ranges = <INTERNAL_REGS INTERNAL_REGS_BASE 0x100000
>                   BOOTROM       BOOTROM_BASE       0x100000
>                   DEVBUS_BOOTCS DEVBUS_BOOTCS_BASE 0x1000000>;
> 

FWIW, I think using macros to pass INTERNAL_REGS_BASE is counterproductive.
It's much better to use literal numbers for those because then you
can see how things fit together by looking at the source.

The macros for the IDs seem fine, although I would actually find it
easier to have just one macro like

#define MBUS_ID(a, b) (((a) & 0xff) << 24 | ((b) & 0xff) << 16)

and pass the literals into that, to the effect of

soc {
	ranges = <1 0 0 0xd0000000 0x100000>,
		 <MBUS_ID(0x01, 0x2f) 0 0 0xfff00000 0x100000>,
		 <MBUS_ID(0x01, 0xe0) 0 0 0xf0000000 0x100000>;

I'd suggest having each macro expand only to a single value, to make
it clearer which numbers belong together in the ranges array.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Grant Likely]

On Tue, 11 Jun 2013 17:26:47 +0200, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Tuesday 11 June 2013 10:57:38 Ezequiel Garcia wrote:
> > Jason, Arnd:
> > 
> > On Sat, Jun 08, 2013 at 07:45:06PM -0600, Jason Gunthorpe wrote:
> > [...]
> > > 
> > > This is the mangling I was referring to. It needs to be the offset
> > > into the target, it can't be something else.
> > > 
> > 
> > I see both of you have explained this already, but I can't seem
> > to catch it yet.
> > 
> > Mind explaining me exaclty *why* "it can't be something else"?
> > IOW, why the window base address cannot be in the 2nd cell,
> > and/or what use cases would this encoding brake?
> 
> Most importantly that would meant that the 'ranges' property
> does not actually reflect the setting in the hardware mbus window,
> which in fact remaps a base address of the upstream bus to address
> zero of the downstream bus, and that address has to be offsettable.
> 
> Taking your example from below:
> 
> ranges = <0x011d0000 0 0xfff00000 0x0100000>; /* BootROM */
> 
> You map the start '0' of the boot rom to 0xfff0000. Your proposal
> would mean setting up the intermediate address space to use the
> same offset that is used on the host side, like 
> 
> ranges = <0x011d0000 0xfff00000 0xfff00000 0x0100000>;
> 
> This essentially you would have to set up the mbus window so the
> bus address subtracts the second cell from the third cell to
> get the child bus address, right?
> 
> One clear example where it would break down is when you have a
> PCI bus window and e.g. map the PCI bus address 0xd0000000 to
> host physical address 0xd0000000. Doing the same calculation
> would mean you need a ranges property like
> 
> ranges = <0x0abc0000 0xa0000000 0xd0000000 0x01000000>;
> 
> so that when setting up the mbus you can subtract 0xd0000000
> from 0xa0000000 and get back (wrapping around through 0) to
> the address 0xd0000000 that you actually want. That would be
> really stupid and illogical.
> 
> Finally, it makes setting up the ranges at boot time extremely
> hard, unless you always want to map each device to the same
> physical address (which is not always possible), because you
> would have to not only change the ranges property of the mbus
> node when you change the window, but also every single 'reg'
> or 'ranges' property in the children referring to it.
> 
> > > I understand your motivation, but this is not a good method of
> > > encoding this in DT.
> > > 
> > > There is nothing especially wrong with updating the ranges at runtime,
> > > but don't use the 2nd cell in the 2dw address to encode the desired
> > > base address.
> > > 
> > 
> > Regarding this, let me confirm your proposed layout using an example.
> > (later on, i'll try the preprocessor on this to make it look better).
> > 
> > mbus {
> > 	compatible = "foo-mbus";
> > 	ranges = < 0xffff0001 0 0xf1000000 0x1000000 /* Internal register entry */
> > 		   0x011d0000 0 0xfff00000 0x0100000 /* BootROM entry with base address */
> > 		   0x012f0000 0 0xe8000000 0x1000000 /* DevBus,NOR entry with base address */
> > 	>;
> > 
> > 	bootrom {
> > 		ranges = <0 0x011d0000 0 0x100000>;
> > 	};
> > 
> > 	devbus-bootcs {
> > 		ranges = <0 0x012f0000 0 0x1000000>;
> > 		nor {
> > 			reg = <0 0x1000000>;
> > 		};
> > 	};
> > };
> > 
> > Is the above correct?
> 
> That looks ok to me, yes. If you have just one device under some of the nodes
> however, I think it's easier use an empty ranges property and do
> 
>  	devbus-bootcs {
>  		ranges;
>  		nor {
>  			reg = <0x012f0000 0 0x1000000>;
>  		};
> 
> The 'ranges' here is most useful if you have multiple devices
> add different offsets. I would also predefine those ranges to
> be as large as possible so you don't have to adapt them when
> the child device grows beyond it.
> 
> > Now, if the above is OK, then the DT looks complete to me. Meaning:
> > 
> > 1. There's no need to update any ranges property at runtime.
> 
> You still have to update the ranges every time you add, remove
> or update a mapping window.
> 
> > 2. There's no need to use an algorithm to 'find' a base address for decoding
> >    windows (such as first-fit as Arnd suggested in another mail).
> >    The base address is already there in the ranges property, so the mbus can
> >    simply allocate the window on that base address.
> > 
> > So, am I right or completely lost?
> 
> Since you already need code to update the ranges property, I think
> it's best to discard the ranges at boot time and create a new mapping
> that just maps the devices you actually want to use. That would make
> it much easier to cope with boot loaders that don't set up the
> mappings in the same way that is listed in the device tree. Just
> put the one entry for the internal-regs in the ranges, since that
> is needed to map do the mbus setup.

It actually seems a bit silly to put the internal regs into the ranges
property at all. It's not like they need to be translated or provided to
any child nodes. Just give the root node a reg property with the correct
base for the internal regs.

As for regenerating the ranges; I have no problem with the kernel
allocating ranges at runtime, but that code should not be creating a new
ranges property and adding it to the tree. The knowledge should be kept
internal to the driver and it should use an of_bus translator
(drivers/of/address.c) to tap into the ranges parsings code.

g.

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 12 June 2013 12:07:46 Grant Likely wrote:
> 
> It actually seems a bit silly to put the internal regs into the ranges
> property at all. It's not like they need to be translated or provided to
> any child nodes. Just give the root node a reg property with the correct
> base for the internal regs.

I think you are missing some background about the internal-regs:

* These are SoC-wide registers including the UART and other things, not
  just the mbus setup
* There are at least two different values used for the internal-regs
  mapping address depending on the SoC and boot loader version
* We have to get the address from the DT since it is impossible to
  probe or change without knowing the current setting: The internal-regs
  also contains the registers in mbus used to relocate the internal-regs.

> As for regenerating the ranges; I have no problem with the kernel
> allocating ranges at runtime, but that code should not be creating a new
> ranges property and adding it to the tree. The knowledge should be kept
> internal to the driver and it should use an of_bus translator
> (drivers/of/address.c) to tap into the ranges parsings code.

Ok, makes sense. I didn't know about the of_bus code. Thanks for the
pointer!

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Grant Likely]

On Wed, Jun 12, 2013 at 12:43 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 12 June 2013 12:07:46 Grant Likely wrote:
>>
>> It actually seems a bit silly to put the internal regs into the ranges
>> property at all. It's not like they need to be translated or provided to
>> any child nodes. Just give the root node a reg property with the correct
>> base for the internal regs.
>
> I think you are missing some background about the internal-regs:

True, I came into this conversation late.

> * These are SoC-wide registers including the UART and other things, not
>   just the mbus setup
> * There are at least two different values used for the internal-regs
>   mapping address depending on the SoC and boot loader version
> * We have to get the address from the DT since it is impossible to
>   probe or change without knowing the current setting: The internal-regs
>   also contains the registers in mbus used to relocate the internal-regs.

I wasn't actually suggesting to not get the address from DT, but your
point is taken; there is a lot more to internal regs than I had
assumed.

>
>> As for regenerating the ranges; I have no problem with the kernel
>> allocating ranges at runtime, but that code should not be creating a new
>> ranges property and adding it to the tree. The knowledge should be kept
>> internal to the driver and it should use an of_bus translator
>> (drivers/of/address.c) to tap into the ranges parsings code.
>
> Ok, makes sense. I didn't know about the of_bus code. Thanks for the
> pointer!
>
>         Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Wednesday 12 June 2013 12:54:59 Grant Likely wrote:
> 
> > * These are SoC-wide registers including the UART and other things, not
> >   just the mbus setup
> > * There are at least two different values used for the internal-regs
> >   mapping address depending on the SoC and boot loader version
> > * We have to get the address from the DT since it is impossible to
> >   probe or change without knowing the current setting: The internal-regs
> >   also contains the registers in mbus used to relocate the internal-regs.
> 
> I wasn't actually suggesting to not get the address from DT, but your
> point is taken; there is a lot more to internal regs than I had
> assumed.

Right, the first point I think is the important one, the rest was
just information added in. At the moment, we actually hardcode the
address in the kernel, but that is changing.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Grant Likely]

On Sat, 8 Jun 2013 19:45:06 -0600, Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Sat, Jun 08, 2013 at 03:38:52PM -0300, Ezequiel Garcia wrote:
> 
> > > mbus {
> > >    ranges = <0x012f0000 0  0xe8000000  0x8000000>
> > >    devbus-bootcs {
> > >        ranges = <0  0x012f0000 0  0x8000000>
> > >    }
> > > }
> > > 
> > > We shouldn't mangle the DT format just to make it convenient for
> > > humans to write - if this is a major problem then I'd try to use the
> > > preprocessor first.. There are several reasonable solutions down that
> > > path, IMHO.
> > > 
> > 
> > Right. I think we have two options here for laying the DT ranges.
> > 
> > 1) This is the proposal implied in the patchset I sent:
> > 
> > mbus {
> > 	ranges = < we only put the internal-reg translation here>
> > 	devbus-bootcs {
> > 		ranges = <0 {target_id/attribute}
> > 	{window_physical_base} {size}>
>         ^^^^^^^^^^^^^^^^^^^^^^
> 
> This is the mangling I was referring to. It needs to be the offset
> into the target, it can't be something else.
> 
> I understand your motivation, but this is not a good method of
> encoding this in DT.
> 
> There is nothing especially wrong with updating the ranges at runtime,
> but don't use the 2nd cell in the 2dw address to encode the desired
> base address.

+1 The design intent of the ranges property is it isolates the parent
address space from the child address space. Encoding the physical
address into the second cell breaks that isolation. Sure, you can do it
and it will work, but then it forces a change to the parent ranges to
propagate down to all the child reg and ranges properties.

g.

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Saturday 08 June 2013 15:38:52 Ezequiel Garcia wrote:
> On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:

> Right. I think we have two options here for laying the DT ranges.
> 
> 1) This is the proposal implied in the patchset I sent:
> 
> mbus {
> 	ranges = < we only put the internal-reg translation here>
> 	devbus-bootcs {
> 		ranges = <0 {target_id/attribute} {window_physical_base} {size}>
> 	}
> }

As Jason explained, you cannot have the window_physical_base in the child
device, that just wouldn't work. I don't know if that's a typo or a thinko ;-)

I also think {size} above there would just be 0xffffffff, right?

> 2) This is what Jason is proposing in his mail:
> 
> mbus {
> 	ranges = <{target_id/attribute} 0 {window_physical_base} {size}>
> 	devbus-bootcs {
> 		ranges = <0 {target_id/attribute} 0 {size}>
> 	}
> }
> 
> Of course this looks much cleaner, but it forces a lot of duplication
> in the DT files. Now, if you see some of the recent patches we've been
> sending, I think this duplication is very error-prone, and it'll be a
> nightmare to maintain. Let me propose an example to show this
> duplication:

I don't see where that duplication comes in. The ranges in the
"devbus-bootcs" are just describing how the hardware maps the
child address space into the global mbus space, and that never
changes. For the cases that only have a single device underneath,
you can actually put an empty ranges property in there and adapt
the "regs" property of whatever sits underneath it. These two
representations would do exactly the same for instance:

a)

mbus {
	ranges = <...>;
	devbus-bootcs {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0 MBUS_BOOTCS 0 0xffffffff>;

		flash {
			regs = <0 0x100000>;
		};
	};
};

mbus {
	ranges = <...>;
	devbus-bootcs {
		#address-cells = <2>;
		#size-cells = <1>;
		ranges;

		flash {
			regs = <MBUS_BOOTCS 0 0x100000>;
		};
	};
};

> Let's suppose we have a board "A" with its armada-A.dts,
> and a common one armada.dtsi.
> 
> The common dtsi file would have this ranges property:
> 
> /* armada.dtsi */
> mbus {
> 	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
> 		         bootrom_id 0       bootrom_base       bootrom_size >
> }
> 
> The A board has a NOR connected to some devbus, so we need to add it
> to the ranges, but also need to duplicate the ones in the common dtsi:
> 
> /* armada-A.dts */
> mbus {
> 	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
> 		         bootrom_id 0       bootrom_base       bootrom_size
> 		         devbus0_id 0       devbus0_base       devbus0_size >
> }

I think the mbus ranges property is best left only in the
board specific .dts file, since you don't know if all of the
mappings are actually set up to the same value by all boot
loaders. In order to avoid a situation where the mbus ranges
describes a setting that is not actually programmed into the
mbus controller by the boot loader, I would leave that empty
by default and only fill it when the OS actually assigns
a mapping.
 
> Now, if we add something at the common level, and extend the ranges
> property in the common armada.dtsi, we also have to go through *each* of
> the per-board dts files (for *each* board) adding that entry, because
> entries *need* to be duplicated. Otherwise you're effectively
> "shadowing" the entries.

We can't really do that anyway, as that would imply we also change
all the boot loaders that have been deployed. I mentioned earlier that
we could also have the mappings we /want/ described in the DT rather the
ones that are set up, but after the discussion about the 0xd0/0xf1
window for the internal registers I think it's better to keep them
in sync all the time. We can leave out mappings here that are set
up but I'd prefer not to put mappings in there that are actually
incorrect.

When assigning the mappings, it's best to just go through all devices
sitting below the mbus and pick an appropriate address for each
'reg' value that gets put into the mbus hardware and into the ranges
property at the same time. The easiest algorithm would be to do
a 'first fit' starting at the highest address that is not already
occupied. If we need the physical address space to be more compact,
we can also first sort all the resources by size. The simpler approach
wastes at most the size difference between the smallest and the largest
range, and that is probably good enough.

I thought this was actually what you implemented already, but it seems
I was wrong.

	Arnd 

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

On Sun, Jun 09, 2013 at 03:42:24PM +0200, Arnd Bergmann wrote:
> On Saturday 08 June 2013 15:38:52 Ezequiel Garcia wrote:
> > On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
> 
> > Right. I think we have two options here for laying the DT ranges.
> > 
> > 1) This is the proposal implied in the patchset I sent:
> > 
> > mbus {
> > 	ranges = < we only put the internal-reg translation here>
> > 	devbus-bootcs {
> > 		ranges = <0 {target_id/attribute} {window_physical_base} {size}>
> > 	}
> > }
> 
> As Jason explained, you cannot have the window_physical_base in the child
> device, that just wouldn't work. I don't know if that's a typo or a thinko ;-)
> 

I'm not sure what you mean by "that just wouldn't work". I understand it
may be a crappy DT layout, but it definitely works.

The proposal I've sent in this patchset has been fully tested and works
in A370 and AXP, with NOR, and PCIe devices. 
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Sunday 09 June 2013 11:34:24 Ezequiel Garcia wrote:
> On Sun, Jun 09, 2013 at 03:42:24PM +0200, Arnd Bergmann wrote:
> > On Saturday 08 June 2013 15:38:52 Ezequiel Garcia wrote:
> > > On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
> > 
> > > Right. I think we have two options here for laying the DT ranges.
> > > 
> > > 1) This is the proposal implied in the patchset I sent:
> > > 
> > > mbus {
> > >     ranges = < we only put the internal-reg translation here>
> > >     devbus-bootcs {
> > >             ranges = <0 {target_id/attribute} {window_physical_base} {size}>
> > >     }
> > > }
> > 
> > As Jason explained, you cannot have the window_physical_base in the child
> > device, that just wouldn't work. I don't know if that's a typo or a thinko 
> > 
> 
> I'm not sure what you mean by "that just wouldn't work". I understand it
> may be a crappy DT layout, but it definitely works.

I didn't mean to imply that you hadn't tested it. I guess it works fine
if you put the same window_physical_base address into both the source
and destination side mbus ranges property, but then you have to pass
it three times in total and it gets really messy when you reassign it
to a different physical address.

	Arnd

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Grant Likely]

On Sat, 8 Jun 2013 15:38:52 -0300, Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Jason, Arnd:
> 
> Thanks for your reviews!
> 
> I agree with most of your suggestions so far. However, I'd like to discuss
> one point before we move forward with the other (imo, less importants)
> issues. See below.
> 
> On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
> [...]
> > 
> > 
> > Is the ranges right though? I was expecting this:
> > 
> >  ranges = <0  0x012f0000 0  0x8000000>
> > 
> > The 2nd address cell in the 2dword space should almost always be 0.
> > 
> > The 2nd address cell should be interprited as the offset within the
> > target's window, not as some kind of physical base address.
> > 
> > >>+ (Note that the windowid cell is encoding the target ID = 0x01 and attribute
> > >>+ ID = 0x2f, and the selected base address for the window is 0xe8000000).
> > 
> > ... The proper place to indicate the base address for the window is in
> > the mbus ranges:
> > 
> > mbus {
> >    ranges = <0x012f0000 0  0xe8000000  0x8000000>
> >    devbus-bootcs {
> >        ranges = <0  0x012f0000 0  0x8000000>
> >    }
> > }
> > 
> > We shouldn't mangle the DT format just to make it convenient for
> > humans to write - if this is a major problem then I'd try to use the
> > preprocessor first.. There are several reasonable solutions down that
> > path, IMHO.
> > 
> 
> Right. I think we have two options here for laying the DT ranges.
> 
> 1) This is the proposal implied in the patchset I sent:
> 
> mbus {
> 	ranges = < we only put the internal-reg translation here>
> 	devbus-bootcs {
> 		ranges = <0 {target_id/attribute} {window_physical_base} {size}>
> 	}
> }
> 
> Of course the above DT will be actually incomplete, for it'll lack a proper ranges
> entry to translate the devbus-bootcs address. So we chosed to do it dynamically
> in the mbus driver (see patch 05/14), and add the missing entry.
> 
> The information of the physical window base address is in this case in
> each child (devbus-bootcs, bootrom, and so on). The MBus driver walks
> each of its first-level children and allocates the window based on the
> address declared in the ranges property of each child, as shown above.
> 
> This is done mostly to avoid having that in the mbus node, and the nightmare
> to maintain it produces. See below.
> 
> 2) This is what Jason is proposing in his mail:
> 
> mbus {
> 	ranges = <{target_id/attribute} 0 {window_physical_base} {size}>
> 	devbus-bootcs {
> 		ranges = <0 {target_id/attribute} 0 {size}>
> 	}
> }
> 
> Of course this looks much cleaner, but it forces a lot of duplication
> in the DT files. Now, if you see some of the recent patches we've been
> sending, I think this duplication is very error-prone, and it'll be a
> nightmare to maintain. Let me propose an example to show this
> duplication:
> 
> Let's suppose we have a board "A" with its armada-A.dts,
> and a common one armada.dtsi.
> 
> The common dtsi file would have this ranges property:
> 
> /* armada.dtsi */
> mbus {
> 	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
> 		         bootrom_id 0       bootrom_base       bootrom_size >
> }
> 
> The A board has a NOR connected to some devbus, so we need to add it
> to the ranges, but also need to duplicate the ones in the common dtsi:
> 
> /* armada-A.dts */
> mbus {
> 	ranges = < internal_regs_id 0 internal_regs_base internal_regs_size
> 		         bootrom_id 0       bootrom_base       bootrom_size
> 		         devbus0_id 0       devbus0_base       devbus0_size >
> }
> 
> Now, if we add something at the common level, and extend the ranges
> property in the common armada.dtsi, we also have to go through *each* of
> the per-board dts files (for *each* board) adding that entry, because
> entries *need* to be duplicated. Otherwise you're effectively
> "shadowing" the entries.
> 
> It is precisely for this reason that I've decided to adopt option #1
> instead! Now, I'm not saying I like that option particularly.
> In fact it has a couple issues as well:
> 
>   1. The DT is *incomplete* and needs to be completed by the MBus
>      driver which, IMHO, sucks.
> 
>   2. Changing the DT dynamically in the kernel, means that new
>      properties are allocated to replace old ones, but the old ones
>      are *never* released. So if for any reason we do this often,
>      we're effectively "leaking" memory.

3. Modify DTC to support appending to properties.

g.

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Ezequiel Garcia]

Hi Arnd,

I have another question!

On Fri, Jun 07, 2013 at 09:01:44PM +0200, Arnd Bergmann wrote:
[...]
> > +Example:
> > +
> > +	soc {
> > +		compatible = "marvell,armada370-mbus", "simple-bus";
> > +		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
> > +	};
> > +
> > +** How does it work?
> > +
> > +The MBus driver controls the allocation and release of the addresses
> > +decoding windows needed to access devices.
> > +
> > +Each window, is identified by its target ID and attribute ID. In order
> > +to represent this, each of mbus-node first-level child has to declare
> > +a suitable 'ranges' translation entry for the MBus to allocate a window
> > +for it. This entry will encode the target and attribute in a 'windowid'
> > +ad-hoc cell, like this:
> > +
> > +	soc {
> > +		bootrom {
> > +			ranges = <0 0x01e00000 0xfff00000 0x100000>;
> > +		};
> > +	};
> 
> I think I'm a bit lost here. Is the "soc" node in this example the node
> that is described as compatible="marvell,armada370-mbus"? Maybe expand
> the example a little here to clarify this.
> 

Exactly: that's the mbus-compatible node. I'll clarify a bit the documentation
so this is clearer.

I kept the 'soc' name in the examples as that's the name in the dts files. 
Do you think I should rename to 'mbus' in the dts files?

I haven't done that since that would imply to change all the dts
files.

Thanks for the help,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding

[Arnd Bergmann]

On Tuesday 11 June 2013 10:31:45 Ezequiel Garcia wrote:
> > > +
> > > +   soc {
> > > +           bootrom {
> > > +                   ranges = <0 0x01e00000 0xfff00000 0x100000>;
> > > +           };
> > > +   };
> > 
> > I think I'm a bit lost here. Is the "soc" node in this example the node
> > that is described as compatible="marvell,armada370-mbus"? Maybe expand
> > the example a little here to clarify this.
> > 
> 
> Exactly: that's the mbus-compatible node. I'll clarify a bit the documentation
> so this is clearer.
> 
> I kept the 'soc' name in the examples as that's the name in the dts files. 
> Do you think I should rename to 'mbus' in the dts files?
> 
> I haven't done that since that would imply to change all the dts
> files.

The name of the device node is not very important, so you can leave
it alone if it causes problems, or change it something else if you
find it more appropriate.

	Arnd

[PATCH 05/14] bus: mvebu-mbus: Update the mbus-compatible node's ranges property

[Ezequiel Garcia]

Ideally 'ranges' entry should be added in device tree source files.
However, since those properties do not inherit the entries from included
files it would be a duplication hell, which means a nightmare to
maintain.

So, instead of hardcoding the required translation entries, we update
them programatically, whenever a decoding window is allocated.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/bus/mvebu-mbus.c | 177 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 160 insertions(+), 17 deletions(-)

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index ac4115a..1779f27 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -53,6 +53,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/mbus.h>
 #include <linux/io.h>
@@ -144,6 +145,7 @@ struct mvebu_mbus_state {
 	struct dentry *debugfs_devs;
 	const struct mvebu_mbus_soc_data *soc;
 	int hw_io_coherency;
+	struct device_node *node;
 };
 
 static struct mvebu_mbus_state mbus_state;
@@ -315,29 +317,155 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
 	return 0;
 }
 
+#define WINDOWID(target, attr) ((target << 24) | (attr << 16))
+
+static int mvebu_mbus_update_ranges(u32 base, u32 size, u32 target, u32 attr)
+{
+	int ranges_cells, addr_cells, c_addr_cells, c_size_cells;
+	int ranges_len, ret;
+	const __be32 *prop;
+	void *value;
+	struct property *old, *new;
+	struct device_node *node = mbus_state.node;
+
+	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);
+
+	ranges_cells = c_addr_cells + addr_cells + c_size_cells;
+
+	pr_debug("add translation: base %x, size %x - target %x, attr %x\n",
+		 base, size, target, attr);
+
+	/* DT nodes always have ranges property */
+	old = of_find_property(node, "ranges", &ranges_len);
+	if (!old)
+		return -EINVAL;
+
+	of_node_get(node);
+
+	/* Allocate new property */
+	new = kzalloc(sizeof(struct property), GFP_KERNEL);
+	if (!new) {
+		ret = -ENOMEM;
+		goto out_put_node;
+	}
+
+	new->name = "ranges";
+	value = kzalloc(old->length + (ranges_cells * sizeof(__be32)),
+			GFP_KERNEL);
+	if (!value) {
+		ret = -ENOMEM;
+		goto out_free_prop;
+	}
+
+	new->value = value;
+	new->length = old->length + ranges_cells * sizeof(__be32);
+	if (old->length) {
+		memcpy(value, old->value, old->length);
+		value = value + old->length;
+	}
+
+	/* Sanity check */
+	if (c_addr_cells != 2 && addr_cells > 2) {
+		ret = -EINVAL;
+		goto out_free_value;
+	}
+
+	/*
+	 * The new ranges entry will look like this:
+	 * <windowid child_base {0} base size>
+	 *
+	 * The extra zeroed-cell is needed if the addressing
+	 * is 64-bit wide.
+	 */
+
+	if (addr_cells > 1) {
+		struct {
+			__be32 windowid;
+			__be32 child_base;
+			__be32 base_high;
+			__be32 base_low;
+			__be32 size;
+		} *r = value;
+
+		r->windowid   = cpu_to_be32(WINDOWID(target, attr));
+		r->child_base = cpu_to_be32(base);
+		r->base_high  = cpu_to_be32(0);
+		r->base_low   = cpu_to_be32(base);
+		r->size       = cpu_to_be32(size);
+	} else {
+		struct {
+			__be32 windowid;
+			__be32 child_base;
+			__be32 base;
+			__be32 size;
+		} *r = value;
+
+		r->windowid   = cpu_to_be32(WINDOWID(target, attr));
+		r->child_base = cpu_to_be32(base);
+		r->base       = cpu_to_be32(base);
+		r->size       = cpu_to_be32(size);
+	}
+
+	ret = of_update_property(node, new);
+	if (ret < 0)
+		goto out_free_value;
+
+	of_node_put(node);
+	return 0;
+
+out_free_value:
+	kfree(new->value);
+out_free_prop:
+	kfree(new);
+out_put_node:
+	of_node_put(node);
+	return ret;
+}
+
 static int mvebu_mbus_alloc_window(struct mvebu_mbus_state *mbus,
 				   phys_addr_t base, size_t size,
 				   phys_addr_t remap, u8 target,
 				   u8 attr)
 {
-	int win;
+	int win, ret = -ENOMEM;
+	bool found = false;
 
 	if (remap == MVEBU_MBUS_NO_REMAP) {
 		for (win = mbus->soc->num_remappable_wins;
 		     win < mbus->soc->num_wins; win++)
-			if (mvebu_mbus_window_is_free(mbus, win))
-				return mvebu_mbus_setup_window(mbus, win, base,
-							       size, remap,
-							       target, attr);
+			if (mvebu_mbus_window_is_free(mbus, win)) {
+				found = true;
+				break;
+			}
 	}
 
+	/*
+	 * Either what we need is a remappable window, or a
+	 * non-remappable window but no such window is available so we
+	 * fallback to using a remappable window.
+	 */
+	if (!found) {
+		for (win = 0; win < mbus->soc->num_wins; win++) {
+			if (mvebu_mbus_window_is_free(mbus, win)) {
+				found = true;
+				break;
+			}
+		}
+	}
 
-	for (win = 0; win < mbus->soc->num_wins; win++)
-		if (mvebu_mbus_window_is_free(mbus, win))
-			return mvebu_mbus_setup_window(mbus, win, base, size,
-						       remap, target, attr);
+	/* No window found */
+	if (!found)
+		return -ENOMEM;
 
-	return -ENOMEM;
+	ret = mvebu_mbus_setup_window(mbus, win, base, size, remap,
+				      target, attr);
+	if (ret < 0)
+		return ret;
+	return mvebu_mbus_update_ranges(base, size, target, attr);
 }
 
 /*
@@ -902,11 +1030,26 @@ static int __init mbus_dt_setup_win(struct mvebu_mbus_state *mbus,
 	const char *name;
 	int i;
 
-	/* Special case for the identity mapping */
-	if (target == 0xff && attr == 0xff)
+	/*
+	 * The identity mapping needs a translation entry at the
+	 * mbus node level. The entry makes this translation:
+	 *   {target/attr base} -> {base}
+	 *
+	 * In other words, it maps the fictitious address space
+	 * that contains a field to encode the target/attribute tuple,
+	 * into the 'true' MBus address space.
+	 */
+	if (target == 0xff && attr == 0xff) {
+		mvebu_mbus_update_ranges(base, size, target, attr);
 		return 0;
+	}
 
-	/* Special case for the internal registers */
+	/*
+	 * Special case for the internal registers:
+	 * There's nothing to do in this case, since
+	 * the internal-regs base address is specified in the
+	 * DT by means of an address translation entry.
+	 */
 	if (target == 0x00 && attr == 0x00)
 		return 0;
 
@@ -938,8 +1081,7 @@ static int __init mbus_dt_setup_win(struct mvebu_mbus_state *mbus,
 	return 0;
 }
 
-static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus,
-				struct device_node *np)
+static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus)
 {
 	struct device_node *child;
 	int ranges_len, tuple_len;
@@ -947,7 +1089,7 @@ static int __init mbus_dt_setup(struct mvebu_mbus_state *mbus,
 	int cell_count;
 	const __be32 *r, *ranges_start, *ranges_end, *prop;
 
-	for_each_available_child_of_node(np, child) {
+	for_each_available_child_of_node(mbus_state.node, child) {
 
 		addr_cells = of_n_addr_cells(child);
 
@@ -1004,6 +1146,7 @@ int __init mvebu_mbus_dt_init(void)
 
 	of_id = of_match_node(of_mvebu_mbus_ids, np);
 	mbus_state.soc = of_id->data;
+	mbus_state.node = np;
 
 	if (of_address_to_resource(np, 0, &mbuswins_res)) {
 		pr_err("cannot get MBUS register address\n");
@@ -1024,6 +1167,6 @@ int __init mvebu_mbus_dt_init(void)
 		return ret;
 
 	/* Setup statically declared windows in the DT */
-	return mbus_dt_setup(&mbus_state, np);
+	return mbus_dt_setup(&mbus_state);
 }
 #endif
-- 
1.8.1.5

Re: [PATCH 05/14] bus: mvebu-mbus: Update the mbus-compatible node's ranges property

[Grant Likely]

On Fri,  7 Jun 2013 13:47:42 -0300, Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Ideally 'ranges' entry should be added in device tree source files.
> However, since those properties do not inherit the entries from included
> files it would be a duplication hell, which means a nightmare to
> maintain.
> 
> So, instead of hardcoding the required translation entries, we update
> them programatically, whenever a decoding window is allocated.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

I'm in the middle of reading through this thread, and I'll have some
better comments for you then, but in the mean time I wanted to reply to
this patch.

As far as the kernel is concerned, the DT is *not* a live structure. It
is a static description of the system.  There are cases where we do
runtime updates to the tree, but those are restricted to either fixups
for broken trees, or because firmware told us to at runtime. Modifying
the data in the tree out of convenience has all kinds of problems.
Rather, the data in the tree should make it clear that there is a gap in
the translation from one node to another and defer to the bus driver to
provide a custom translation when requested without modifying the tree.

g.

[PATCH 06/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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 07/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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 08/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 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 09/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370-xp.dtsi             | 6 ++----
 arch/arm/boot/dts/armada-370.dtsi                | 6 ++++--
 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                 | 4 ++++
 5 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 52a1f5e..68d9fa1 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -29,18 +29,16 @@
 	};
 
 	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 0 0 0x100000>;
 
 			mpic: interrupt-controller@20000 {
 				compatible = "marvell,mpic";
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index aee2b18..54cd3ef 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -29,8 +29,10 @@
 	};
 
 	soc {
-		ranges = <0          0xd0000000 0x0100000 /* internal registers */
-			  0xe0000000 0xe0000000 0x8100000 /* PCIe */>;
+		compatible = "marvell,armada370-mbus", "simple-bus";
+		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
+		ranges = <0 0 0xd0000000 0x100000>;
+
 		internal-regs {
 			system-controller@18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 3ee63d1..26ad06f 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -39,9 +39,6 @@
 	};
 
 	soc {
-		ranges = <0          0 0xd0000000 0x100000
-			  0xf0000000 0 0xf0000000 0x1000000>;
-
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
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..f14d36c 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -27,9 +27,6 @@
 	};
 
 	soc {
-		ranges = <0          0 0xd0000000 0x100000
-			  0xf0000000 0 0xf0000000 0x8000000>;
-
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 1ee8540..a3865f7 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -23,6 +23,10 @@
 	compatible = "marvell,armadaxp", "marvell,armada-370-xp";
 
 	soc {
+		compatible = "marvell,armadaxp-mbus", "simple-bus";
+		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
+		ranges = <0 0 0 0xd0000000 0x100000>;
+
 		internal-regs {
 			L2: l2-cache {
 				compatible = "marvell,aurora-system-cache";
-- 
1.8.1.5

[PATCH 10/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370.dtsi | 6 ++++++
 arch/arm/boot/dts/armada-xp.dtsi  | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 54cd3ef..1b11f1f 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -33,6 +33,12 @@
 		reg = <0xd0020000 0x100>, <0xd0020180 0x20>;
 		ranges = <0 0 0xd0000000 0x100000>;
 
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x01e00000 0xfff00000 0x100000>;
+		};
+
 		internal-regs {
 			system-controller@18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index a3865f7..5a26d81 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -27,6 +27,12 @@
 		reg = <0 0xd0020000 0 0x100>, <0 0xd0020180 0 0x20>;
 		ranges = <0 0 0 0xd0000000 0x100000>;
 
+		bootrom {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x011d0000 0xfff00000 0x100000>;
+		};
+
 		internal-regs {
 			L2: l2-cache {
 				compatible = "marvell,aurora-system-cache";
-- 
1.8.1.5

[PATCH 11/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370-xp.dtsi             | 90 ++++++++++++------------
 arch/arm/boot/dts/armada-xp-gp.dts               | 58 +++++++--------
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 58 +++++++--------
 3 files changed, 103 insertions(+), 103 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 68d9fa1..15b558a 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -34,6 +34,51 @@
 		compatible = "simple-bus";
 		interrupt-parent = <&mpic>;
 
+		devbus-bootcs {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0 0x10400 0x8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs0 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0 0x10408 0x8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs1 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0 0x10410 0x8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs2 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0 0x10418 0x8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs3 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0 0x10420 0x8>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
 		internal-regs {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -179,51 +224,6 @@
 				clocks = <&coreclk 0>;
 				status = "disabled";
 			};
-
-			devbus-bootcs@10400 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10400 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs0@10408 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10408 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs1@10410 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10410 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs2@10418 {
-				compatible = "marvell,mvebu-devbus";
-				reg = <0x10418 0x8>;
-				#address-cells = <1>;
-				#size-cells = <1>;
-				clocks = <&coreclk 0>;
-				status = "disabled";
-			};
-
-			devbus-cs3@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 26ad06f..d045d6e 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -39,6 +39,35 @@
 	};
 
 	soc {
+		devbus-bootcs {
+			status = "okay";
+			ranges = <0 0x012f0000 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@0 {
+				compatible = "cfi-flash";
+				reg = <0 0x1000000>;
+				bank-width = <2>;
+			};
+		};
+
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
@@ -113,35 +142,6 @@
 				};
 			};
 
-			devbus-bootcs@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@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 f14d36c..a895e1b 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -27,6 +27,35 @@
 	};
 
 	soc {
+		devbus-bootcs {
+			status = "okay";
+			ranges = <0 0x012f0000 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@0 {
+				compatible = "cfi-flash";
+				reg = <0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
+
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
@@ -141,35 +170,6 @@
 				status = "okay";
 			};
 
-			devbus-bootcs@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@0 {
-					compatible = "cfi-flash";
-					reg = <0 0x8000000>;
-					bank-width = <2>;
-				};
-			};
-
 			pcie-controller {
 				status = "okay";
 				/* Internal mini-PCIe connector */
-- 
1.8.1.5

Re: [PATCH 11/14] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes

[Arnd Bergmann]

On Friday 07 June 2013, Ezequiel Garcia wrote:
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -34,6 +34,51 @@
>                 compatible = "simple-bus";
>                 interrupt-parent = <&mpic>;
>  
> +               devbus-bootcs {
> +                       compatible = "marvell,mvebu-devbus";
> +                       reg = <0 0x10400 0x8>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       clocks = <&coreclk 0>;
> +                       status = "disabled";
> +               };
> +
...
>                internal-regs {
>                        compatible = "simple-bus";
>                        #address-cells = <1>;


I think you are missing a couple of things here:

* The mbus node needs #address-cells and #size-cells, and is no longer
  compatible="simple-bus"m since it's actually complex.

* The devbus-bootcs node and all other ones need "ranges" properties.

* The "reg" property of the devbus-bootcs device needs to be in
  mbus translated address space, so I assume the first cell would
  be something other than '0'.

* With the proposal I made for patch 3, you would actually
  need a ranges property for the mbus node as well.

	Arnd

[PATCH 12/14] ARM: mvebu: Remove device tree unused properties on A370

[Ezequiel Garcia]

These properties are not needed so it's safe to remove them.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370.dtsi | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 1b11f1f..4fd483f 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -188,10 +188,6 @@
 
 				bus-range = <0x00 0xff>;
 
-				reg = <0x40000 0x2000>, <0x80000 0x2000>;
-
-				reg-names = "pcie0.0", "pcie1.0";
-
 				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 */
-- 
1.8.1.5

Re: [PATCH 12/14] ARM: mvebu: Remove device tree unused properties on A370

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Fri,  7 Jun 2013 13:47:49 -0300, Ezequiel Garcia wrote:
> These properties are not needed so it's safe to remove them.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

I believe this one should be applied now, regardless of what happens to
the rest of the patch series.

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

Re: [PATCH 12/14] ARM: mvebu: Remove device tree unused properties on A370

[Jason Cooper]

On Fri, Jun 07, 2013 at 01:47:49PM -0300, Ezequiel Garcia wrote:
> These properties are not needed so it's safe to remove them.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/armada-370.dtsi | 4 ----
>  1 file changed, 4 deletions(-)

Also as mentioned by Thomas, applied to mvebu/dt with his Ack.

thx,

Jason.

[PATCH 13/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370-mirabox.dts | 32 +++++------
 arch/arm/boot/dts/armada-370.dtsi        | 97 ++++++++++++++++----------------
 2 files changed, 65 insertions(+), 64 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts
index 14e36e1..dcc33e3 100644
--- a/arch/arm/boot/dts/armada-370-mirabox.dts
+++ b/arch/arm/boot/dts/armada-370-mirabox.dts
@@ -25,6 +25,22 @@
 	};
 
 	soc {
+		pcie-controller {
+			status = "okay";
+
+			/* Internal mini-PCIe connector */
+			pcie@1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+
+			/* Connected on the PCB to a USB 3.0 XHCI controller */
+			pcie@2,0 {
+				/* Port 1, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <200000000>;
@@ -119,22 +135,6 @@
 					reg = <0x25>;
 				};
 			};
-
-			pcie-controller {
-				status = "okay";
-
-				/* Internal mini-PCIe connector */
-				pcie@1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-
-				/* Connected on the PCB to a USB 3.0 XHCI controller */
-				pcie@2,0 {
-					/* Port 1, Lane 0 */
-					status = "okay";
-				};
-			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 4fd483f..f35efe3 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -39,6 +39,55 @@
 			ranges = <0 0x01e00000 0xfff00000 0x100000>;
 		};
 
+		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    0          0x40000    0 0x00002000
+				0x82000000 0 0x80000    0          0x80000    0 0x00002000
+				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000
+				0x81000000 0 0          0xffff0000 0xe8000000 0 0x00100000>;
+
+			pcie@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@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@18200 {
 				compatible = "marvell,armada-370-xp-system-controller";
@@ -177,54 +226,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@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@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 14/14] 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-xp-db.dts               |  66 ++--
 arch/arm/boot/dts/armada-xp-gp.dts               |  42 +--
 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 |  18 +-
 6 files changed, 480 insertions(+), 477 deletions(-)

diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index d6cc8bf..32e25fe 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -30,6 +30,39 @@
 	};
 
 	soc {
+		pcie-controller {
+			status = "okay";
+
+			/*
+			 * All 6 slots are physically present as
+			 * standard PCIe slots on the board.
+			 */
+			pcie@1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+			pcie@2,0 {
+				/* Port 0, Lane 1 */
+				status = "okay";
+			};
+			pcie@3,0 {
+				/* Port 0, Lane 2 */
+				status = "okay";
+			};
+			pcie@4,0 {
+				/* Port 0, Lane 3 */
+				status = "okay";
+			};
+			pcie@9,0 {
+				/* Port 2, Lane 0 */
+				status = "okay";
+			};
+			pcie@10,0 {
+				/* Port 3, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
@@ -122,39 +155,6 @@
 					spi-max-frequency = <20000000>;
 				};
 			};
-
-			pcie-controller {
-				status = "okay";
-
-				/*
-				 * All 6 slots are physically present as
-				 * standard PCIe slots on the board.
-				 */
-				pcie@1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-				pcie@2,0 {
-					/* Port 0, Lane 1 */
-					status = "okay";
-				};
-				pcie@3,0 {
-					/* Port 0, Lane 2 */
-					status = "okay";
-				};
-				pcie@4,0 {
-					/* Port 0, Lane 3 */
-					status = "okay";
-				};
-				pcie@9,0 {
-					/* Port 2, Lane 0 */
-					status = "okay";
-				};
-				pcie@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 d045d6e..333dd3b 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -68,6 +68,27 @@
 			};
 		};
 
+		pcie-controller {
+			status = "okay";
+
+			/*
+			 * The 3 slots are physically present as
+			 * standard PCIe slots on the board.
+			 */
+			pcie@1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+			pcie@9,0 {
+				/* Port 2, Lane 0 */
+				status = "okay";
+			};
+			pcie@10,0 {
+				/* Port 3, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
@@ -141,27 +162,6 @@
 					spi-max-frequency = <108000000>;
 				};
 			};
-
-			pcie-controller {
-				status = "okay";
-
-				/*
-				 * The 3 slots are physically present as
-				 * standard PCIe slots on the board.
-				 */
-				pcie@1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-				pcie@9,0 {
-					/* Port 2, Lane 0 */
-					status = "okay";
-				};
-				pcie@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..e0265e5 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 0 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0	  0xffff0000 0xe8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie@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@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@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@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@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@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@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@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@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@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 f4029f0..f2bb74b 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 0 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0x80000 0 0x80000 0 0x00002000   /* Port 1.0 registers */
+				0x82000000 0 0x82000 0 0x82000 0 0x00002000   /* Port 3.0 registers */
+				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0	  0xffff0000 0xe8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie@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@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@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@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@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@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@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@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@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@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@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@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 488ca5e..21376c8 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 0 0x40000 0 0x00002000   /* Port 0.0 registers */
+				0x82000000 0 0x42000 0 0x42000 0 0x00002000   /* Port 2.0 registers */
+				0x82000000 0 0x44000 0 0x44000 0 0x00002000   /* Port 0.1 registers */
+				0x82000000 0 0x48000 0 0x48000 0 0x00002000   /* Port 0.2 registers */
+				0x82000000 0 0x4c000 0 0x4c000 0 0x00002000   /* Port 0.3 registers */
+				0x82000000 0 0x80000 0 0x80000 0 0x00002000   /* Port 1.0 registers */
+				0x82000000 0 0x82000 0 0x82000 0 0x00002000   /* Port 3.0 registers */
+				0x82000000 0 0x84000 0 0x84000 0 0x00002000   /* Port 1.1 registers */
+				0x82000000 0 0x88000 0 0x88000 0 0x00002000   /* Port 1.2 registers */
+				0x82000000 0 0x8c000 0 0x8c000 0 0x00002000   /* Port 1.3 registers */
+				0x82000000 0 0xe0000000 0xffff0000 0xe0000000 0 0x08000000   /* non-prefetchable memory */
+				0x81000000 0 0	  0xffff0000 0xe8000000 0 0x00100000>; /* downstream I/O */
+
+			pcie@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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@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 a895e1b..d2cb11e 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -56,6 +56,15 @@
 			};
 		};
 
+		pcie-controller {
+			status = "okay";
+			/* Internal mini-PCIe connector */
+			pcie@1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+		};
+
 		internal-regs {
 			serial@12000 {
 				clock-frequency = <250000000>;
@@ -169,15 +178,6 @@
 			usb@51000 {
 				status = "okay";
 			};
-
-			pcie-controller {
-				status = "okay";
-				/* Internal mini-PCIe connector */
-				pcie@1,0 {
-					/* Port 0, Lane 0 */
-					status = "okay";
-				};
-			};
 		};
 	};
 };
-- 
1.8.1.5