MBus device tree binding

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 <[email protected]>

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 <[email protected]>

---
 drivers/bus/mvebu-mbus.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/bus/mvebu-mbus.c +8 -0

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

Quoted 8 lines · Ezequiel Garcia

drivers/bus/mvebu-mbus.c +8 -0

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

drivers/bus/mvebu-mbus.c int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base, · line 912

Quoted 10 lines

drivers/bus/mvebu-mbus.c +10 -0

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

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.

Quoted 5 lines · Arnd Bergmann

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

Quoted 1 line

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.

Quoted 10 lines

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.

Quoted 24 lines · Arnd Bergmann

Quoted 8 lines · Ezequiel Garcia

drivers/bus/mvebu-mbus.c +8 -0

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

drivers/bus/mvebu-mbus.c int __init mvebu_mbus_init(const char *soc, phys_addr_t mbuswins_phys_base, · line 912

Quoted 10 lines

drivers/bus/mvebu-mbus.c +10 -0

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

Quoted 3 lines

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.

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 <[email protected]>

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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +0 -0

new file mode 100644
index 0000000..2b0303e

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +13 -0

+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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +4 -0

+The obvious way is to add a translation at the mbus-node level, like this:
+
+	soc {
+		ranges = <0x01e00000 0xfff00000 0xfff00000 0x100000>;

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +9 -0

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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +11 -0

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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 8

Quoted 3 lines

+- compatible: Should be set to one of the following:

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +2 -0

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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 13

Quoted 4 lines

+- reg: Device's register space.

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +3 -0

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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 37

Quoted 5 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +5 -0

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

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.

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 76

Quoted 4 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +4 -0

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

Quoted 2 lines

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

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?

Quoted 12 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +12 -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 at 1,0 {
+				/* ... */
+			};

Review

Quoted 2 lines

(quoted diff) +2 -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.

Quoted 7 lines · Arnd Bergmann

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 8

Quoted 3 lines

+- compatible: Should be set to one of the following:

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +2 -0

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

Quoted 8 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 76

Quoted 4 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +4 -0

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

Quoted 2 lines

Quoted 2 lines

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

Quoted 4 lines

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

Quoted 12 lines

Quoted 12 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +12 -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 at 1,0 {
+				/* ... */
+			};

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -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.

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 5 lines

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 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:

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:

What do you think?

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

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

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.

Jason, Arnd:

On Sat, Jun 08, 2013 at 07:45:06PM -0600, Jason Gunthorpe wrote:
[...]

Quoted 4 lines

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?

Quoted 7 lines

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?

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

(quoted diff) +0 -0

 	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.

Quoted 6 lines

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.

Quoted 9 lines · Arnd Bergmann

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

Quoted 4 lines

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

Quoted 7 lines

Quoted 6 lines

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?

Quoted 7 lines

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?

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?

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

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?

Quoted 6 lines · Sebastian Hesselbarth

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?

Quoted 9 lines · Arnd Bergmann

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

(quoted diff) +0 -0

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

Quoted 4 lines

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

Quoted 7 lines

Quoted 6 lines

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?

Quoted 7 lines

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?

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.

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 4 lines · Arnd Bergmann

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?

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 2 lines

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.

Quoted 9 lines · 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.

Quoted 2 lines

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

Quoted 6 lines · Grant Likely

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!

Quoted 5 lines

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

Quoted 3 lines

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)?

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

Quoted 5 lines

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.

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.

Quoted 3 lines · Arnd Bergmann

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.

Quoted 5 lines · Ezequiel Garcia

Quoted 5 lines

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??

Quoted 8 lines

Quoted 3 lines

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

Quoted 9 lines · Arnd Bergmann

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

(quoted diff) +0 -0

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

Quoted 4 lines

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 at 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 at 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'm probably missing something, but... are you sure that's supposed to work?

#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>;

Quoted 9 lines · 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:
[...]

Quoted 4 lines

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

Quoted 6 lines

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.

Quoted 30 lines

Quoted 7 lines

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

Quoted 1 line

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

(quoted diff) +0 -0

 	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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 5 lines · 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.

Quoted 6 lines

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

Quoted 8 lines

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

Quoted 15 lines · Ezequiel Garcia

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 7 lines

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.

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}>
	}
}

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.

Quoted 4 lines · 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.

Quoted 8 lines

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 ;-)

Quoted 14 lines · Arnd Bergmann

Quoted 12 lines · 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}>
}
}

Quoted 3 lines

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.

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:
[...]

Quoted 24 lines

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.

Quoted 2 lines

Quoted 2 lines

+ (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>
   }
}

Quoted 5 lines

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.

Quoted 19 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +19 -0

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

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 37

Quoted 4 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +4 -0

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

Quoted 11 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt line 37

Quoted 6 lines

Documentation/devicetree/bindings/bus/mvebu-mbus.txt new file +6 -0

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

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 <[email protected]>

arch/arm/boot/dts/armada-370-xp.dtsi +9 -0

@@ -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>;

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

Signed-off-by Ezequiel Garcia <[email protected]>

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

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370.dtsi | 4 ----
 1 file changed, 4 deletions(-)